refactor: use the proper executable tx as mempool input in gateway

crates/gateway/src/gateway.rs

@@ -113,8 +113,6 @@ fn process_tx(
     // Perform stateless validations.
     stateless_tx_validator.validate(&tx)?;
 
-    // TODO(Arni): remove copy_of_rpc_tx and use executable_tx directly as the mempool input.
-    let copy_of_rpc_tx = tx.clone();
     let executable_tx = compile_contract_and_build_executable_tx(
         tx,
         &gateway_compiler,
@@ -135,11 +133,7 @@ fn process_tx(
 
     // TODO(Arni): Add the Sierra and the Casm to the mempool input.
     Ok(MempoolInput {
-        tx: Transaction::new_from_rpc_tx(
-            copy_of_rpc_tx,
-            validate_info.tx_hash,
-            validate_info.sender_address,
-        ),
+        tx: executable_tx,
         account: Account {
             sender_address: validate_info.sender_address,
             state: AccountState { nonce: validate_info.account_nonce },

crates/gateway/src/gateway_test.rs

@@ -1,49 +1,19 @@
 use std::sync::Arc;
 
 use assert_matches::assert_matches;
-use blockifier::context::ChainInfo;
-use blockifier::execution::contract_class::ClassInfo;
 use blockifier::test_utils::CairoVersion;
-use blockifier::transaction::account_transaction::AccountTransaction;
-use blockifier::transaction::transactions::{
-    DeclareTransaction as BlockifierDeclareTransaction,
-    DeployAccountTransaction as BlockifierDeployAccountTransaction,
-    InvokeTransaction as BlockifierInvokeTransaction,
-};
-use mempool_test_utils::starknet_api_test_utils::{create_executable_tx, declare_tx, invoke_tx};
+use mempool_test_utils::starknet_api_test_utils::{declare_tx, invoke_tx};
 use mockall::predicate::eq;
-use starknet_api::core::{
-    calculate_contract_address,
-    ChainId,
-    ClassHash,
-    CompiledClassHash,
-    ContractAddress,
-};
-use starknet_api::rpc_transaction::{
-    RpcDeclareTransaction,
-    RpcDeployAccountTransaction,
-    RpcInvokeTransaction,
-    RpcTransaction,
-};
-use starknet_api::transaction::{
-    DeclareTransaction,
-    DeclareTransactionV3,
-    DeployAccountTransaction,
-    DeployAccountTransactionV3,
-    InvokeTransactionV3,
-    TransactionHash,
-    TransactionHasher,
-    ValidResourceBounds,
-};
+use starknet_api::core::{ChainId, CompiledClassHash, ContractAddress};
+use starknet_api::executable_transaction::{InvokeTransaction, Transaction};
+use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction};
 use starknet_gateway_types::errors::GatewaySpecError;
 use starknet_mempool_types::communication::{MempoolWrapperInput, MockMempoolClient};
 use starknet_mempool_types::mempool_types::{Account, AccountState, MempoolInput};
 use starknet_sierra_compile::config::SierraToCasmCompilationConfig;
-use tracing::error;
 
 use crate::compilation::GatewayCompiler;
 use crate::config::{StatefulTransactionValidatorConfig, StatelessTransactionValidatorConfig};
-use crate::errors::StatefulTransactionValidatorResult;
 use crate::gateway::{internal_add_tx, AppState, SharedMempoolClient};
 use crate::state_reader_test_utils::{local_test_state_reader_factory, TestStateReaderFactory};
 use crate::stateful_transaction_validator::StatefulTransactionValidator;
@@ -84,33 +54,31 @@ fn create_tx() -> (RpcTransaction, SenderAddress) {
 // TODO: add test with Some broadcasted message metadata
 #[tokio::test]
 async fn test_add_tx() {
-    let (tx, sender_address) = create_tx();
-    let tx_hash = calculate_hash(&tx);
+    let (rpc_tx, sender_address) = create_tx();
+    let rpc_invoke_tx =
+        assert_matches!(rpc_tx.clone(), RpcTransaction::Invoke(rpc_invoke_tx) => rpc_invoke_tx);
+    let executable_tx = Transaction::Invoke(
+        InvokeTransaction::from_rpc_tx(rpc_invoke_tx, &ChainId::create_for_testing()).unwrap(),
+    );
+
+    let tx_hash = executable_tx.tx_hash();
 
     let mut mock_mempool_client = MockMempoolClient::new();
     mock_mempool_client
         .expect_add_tx()
         .once()
         .with(eq(MempoolWrapperInput {
-            // TODO(Arni): Use external_to_executable_tx instead of `create_executable_tx`. Consider
-            // creating a `convertor for testing` that does not do the compilation.
             mempool_input: MempoolInput {
-                tx: create_executable_tx(
-                    sender_address,
-                    tx_hash,
-                    *tx.tip(),
-                    *tx.nonce(),
-                    ValidResourceBounds::AllResources(*tx.resource_bounds()),
-                ),
-                account: Account { sender_address, state: AccountState { nonce: *tx.nonce() } },
+                tx: executable_tx,
+                account: Account { sender_address, state: AccountState { nonce: *rpc_tx.nonce() } },
             },
             message_metadata: None,
         }))
         .return_once(|_| Ok(()));
     let state_reader_factory = local_test_state_reader_factory(CairoVersion::Cairo1, false);
     let app_state = app_state(Arc::new(mock_mempool_client), state_reader_factory);
 
-    let response_tx_hash = internal_add_tx(app_state, tx).await.unwrap();
+    let response_tx_hash = internal_add_tx(app_state, rpc_tx).await.unwrap();
 
     assert_eq!(tx_hash, response_tx_hash);
 }
@@ -133,125 +101,3 @@ async fn test_compiled_class_hash_mismatch() {
     let err = internal_add_tx(app_state, tx).await.unwrap_err();
     assert_matches!(err, GatewaySpecError::CompiledClassHashMismatch);
 }
-
-fn calculate_hash(rpc_tx: &RpcTransaction) -> TransactionHash {
-    let optional_class_info = match &rpc_tx {
-        RpcTransaction::Declare(_declare_tx) => {
-            panic!("Declare transactions are not supported in this test")
-        }
-        _ => None,
-    };
-
-    let account_tx = rpc_tx_to_account_tx(
-        rpc_tx,
-        optional_class_info,
-        &ChainInfo::create_for_testing().chain_id,
-    )
-    .unwrap();
-    account_tx.tx_hash()
-}
-
-// TODO(Arni): Remove this function. Replace with a function that take ownership of RpcTransaction.
-fn rpc_tx_to_account_tx(
-    rpc_tx: &RpcTransaction,
-    // FIXME(yael 15/4/24): calculate class_info inside the function once compilation code is ready
-    optional_class_info: Option<ClassInfo>,
-    chain_id: &ChainId,
-) -> StatefulTransactionValidatorResult<AccountTransaction> {
-    match rpc_tx {
-        RpcTransaction::Declare(RpcDeclareTransaction::V3(tx)) => {
-            let declare_tx = DeclareTransaction::V3(DeclareTransactionV3 {
-                class_hash: ClassHash::default(), /* FIXME(yael 15/4/24): call the starknet-api
-                                                   * function once ready */
-                resource_bounds: ValidResourceBounds::AllResources(
-                    rpc_tx.resource_bounds().clone(),
-                ),
-                tip: tx.tip,
-                signature: tx.signature.clone(),
-                nonce: tx.nonce,
-                compiled_class_hash: tx.compiled_class_hash,
-                sender_address: tx.sender_address,
-                nonce_data_availability_mode: tx.nonce_data_availability_mode,
-                fee_data_availability_mode: tx.fee_data_availability_mode,
-                paymaster_data: tx.paymaster_data.clone(),
-                account_deployment_data: tx.account_deployment_data.clone(),
-            });
-            let tx_hash = declare_tx
-                .calculate_transaction_hash(chain_id, &declare_tx.version())
-                .map_err(|e| {
-                    error!("Failed to calculate tx hash: {}", e);
-                    GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
-                })?;
-            let class_info =
-                optional_class_info.expect("declare transaction should contain class info");
-            let declare_tx = BlockifierDeclareTransaction::new(declare_tx, tx_hash, class_info)
-                .map_err(|e| {
-                    error!("Failed to convert declare tx hash to blockifier tx type: {}", e);
-                    GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
-                })?;
-            Ok(AccountTransaction::Declare(declare_tx))
-        }
-        RpcTransaction::DeployAccount(RpcDeployAccountTransaction::V3(tx)) => {
-            let deploy_account_tx = DeployAccountTransaction::V3(DeployAccountTransactionV3 {
-                resource_bounds: ValidResourceBounds::AllResources(
-                    rpc_tx.resource_bounds().clone(),
-                ),
-                tip: tx.tip,
-                signature: tx.signature.clone(),
-                nonce: tx.nonce,
-                class_hash: tx.class_hash,
-                contract_address_salt: tx.contract_address_salt,
-                constructor_calldata: tx.constructor_calldata.clone(),
-                nonce_data_availability_mode: tx.nonce_data_availability_mode,
-                fee_data_availability_mode: tx.fee_data_availability_mode,
-                paymaster_data: tx.paymaster_data.clone(),
-            });
-            let contract_address = calculate_contract_address(
-                deploy_account_tx.contract_address_salt(),
-                deploy_account_tx.class_hash(),
-                &deploy_account_tx.constructor_calldata(),
-                ContractAddress::default(),
-            )
-            .map_err(|e| {
-                error!("Failed to calculate contract address: {}", e);
-                GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
-            })?;
-            let tx_hash = deploy_account_tx
-                .calculate_transaction_hash(chain_id, &deploy_account_tx.version())
-                .map_err(|e| {
-                    error!("Failed to calculate tx hash: {}", e);
-                    GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
-                })?;
-            let deploy_account_tx = BlockifierDeployAccountTransaction::new(
-                deploy_account_tx,
-                tx_hash,
-                contract_address,
-            );
-            Ok(AccountTransaction::DeployAccount(deploy_account_tx))
-        }
-        RpcTransaction::Invoke(RpcInvokeTransaction::V3(tx)) => {
-            let invoke_tx = starknet_api::transaction::InvokeTransaction::V3(InvokeTransactionV3 {
-                resource_bounds: ValidResourceBounds::AllResources(
-                    rpc_tx.resource_bounds().clone(),
-                ),
-                tip: tx.tip,
-                signature: tx.signature.clone(),
-                nonce: tx.nonce,
-                sender_address: tx.sender_address,
-                calldata: tx.calldata.clone(),
-                nonce_data_availability_mode: tx.nonce_data_availability_mode,
-                fee_data_availability_mode: tx.fee_data_availability_mode,
-                paymaster_data: tx.paymaster_data.clone(),
-                account_deployment_data: tx.account_deployment_data.clone(),
-            });
-            let tx_hash = invoke_tx
-                .calculate_transaction_hash(chain_id, &invoke_tx.version())
-                .map_err(|e| {
-                    error!("Failed to calculate tx hash: {}", e);
-                    GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
-                })?;
-            let invoke_tx = BlockifierInvokeTransaction::new(invoke_tx, tx_hash);
-            Ok(AccountTransaction::Invoke(invoke_tx))
-        }
-    }
-}

crates/gateway/src/stateful_transaction_validator.rs

@@ -23,7 +23,6 @@ use tracing::error;
 use crate::config::StatefulTransactionValidatorConfig;
 use crate::errors::StatefulTransactionValidatorResult;
 use crate::state_reader::{MempoolStateReader, StateReaderFactory};
-use crate::utils::get_sender_address;
 
 #[cfg(test)]
 #[path = "stateful_transaction_validator_test.rs"]
@@ -75,6 +74,9 @@ impl StatefulTransactionValidator {
         executable_tx: &ExecutableTransaction,
         mut validator: V,
     ) -> StatefulTransactionValidatorResult<ValidateInfo> {
+        let tx_hash = executable_tx.tx_hash();
+        let sender_address = executable_tx.contract_address();
+
         let account_tx = AccountTransaction::try_from(
             // TODO(Arni): create a try_from for &ExecutableTransaction.
             executable_tx.clone(),
@@ -83,8 +85,6 @@ impl StatefulTransactionValidator {
             error!("Failed to convert executable transaction into account transaction: {}", error);
             GatewaySpecError::UnexpectedError { data: "Internal server error".to_owned() }
         })?;
-        let tx_hash = account_tx.tx_hash();
-        let sender_address = get_sender_address(&account_tx);
         let account_nonce = validator.get_nonce(sender_address).map_err(|e| {
             error!("Failed to get nonce for sender address {}: {}", sender_address, e);
             GatewaySpecError::UnexpectedError { data: "Internal server error.".to_owned() }
@@ -149,6 +149,7 @@ pub fn get_latest_block_info(
     })
 }
 
+// TODO(Arni): Remove irrelevant fields.
 /// Holds members created by the stateful transaction validator, needed for
 /// [`MempoolInput`](starknet_mempool_types::mempool_types::MempoolInput).
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]

crates/gateway/src/stateful_transaction_validator_test.rs

@@ -6,7 +6,7 @@ use blockifier::context::BlockContext;
 use blockifier::test_utils::CairoVersion;
 use blockifier::transaction::errors::{TransactionFeeError, TransactionPreValidationError};
 use mempool_test_utils::starknet_api_test_utils::{
-    executable_invoke_tx,
+    executable_invoke_tx as create_executable_invoke_tx,
     TEST_SENDER_ADDRESS,
     VALID_L1_GAS_MAX_AMOUNT,
     VALID_L1_GAS_MAX_PRICE_PER_UNIT,
@@ -17,8 +17,10 @@ use pretty_assertions::assert_eq;
 use rstest::{fixture, rstest};
 use starknet_api::core::{ContractAddress, Nonce, PatriciaKey};
 use starknet_api::executable_transaction::Transaction;
+use starknet_api::test_utils::deploy_account::executable_deploy_account_tx;
+use starknet_api::test_utils::invoke::executable_invoke_tx;
 use starknet_api::transaction::{Resource, TransactionHash};
-use starknet_api::{contract_address, felt, patricia_key};
+use starknet_api::{contract_address, deploy_account_tx_args, felt, invoke_tx_args, patricia_key};
 use starknet_gateway_types::errors::GatewaySpecError;
 use starknet_types_core::felt::Felt;
 
@@ -63,14 +65,17 @@ fn stateful_validator(block_context: BlockContext) -> StatefulTransactionValidat
 // TODO(Arni): consider testing declare and deploy account.
 #[rstest]
 #[case::valid_tx(
-    executable_invoke_tx(CairoVersion::Cairo1),
+    create_executable_invoke_tx(CairoVersion::Cairo1),
     Ok(ValidateInfo{
         tx_hash: TransactionHash::default(),
         sender_address: contract_address!("0xc0020000"),
         account_nonce: Nonce::default()
     })
 )]
-#[case::invalid_tx(executable_invoke_tx(CairoVersion::Cairo1), Err(STATEFUL_VALIDATOR_FEE_ERROR))]
+#[case::invalid_tx(
+    create_executable_invoke_tx(CairoVersion::Cairo1),
+    Err(STATEFUL_VALIDATOR_FEE_ERROR)
+)]
 fn test_stateful_tx_validator(
     #[case] executable_tx: Transaction,
     #[case] expected_result: BlockifierStatefulValidatorResult<ValidateInfo>,
@@ -125,30 +130,24 @@ fn test_instantiate_validator() {
 
 #[rstest]
 #[case::should_skip_validation(
-    Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx(
-        starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ONE))
-    )),
+    Transaction::Invoke(executable_invoke_tx(invoke_tx_args!(nonce: Nonce(Felt::ONE)))),
     Nonce::default(),
     true
 )]
 #[case::should_not_skip_validation_nonce_over_max_nonce_for_skip(
-    Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx(
-        starknet_api::invoke_tx_args!(nonce: Nonce(Felt::ZERO))
-    )),
+    Transaction::Invoke(executable_invoke_tx(invoke_tx_args!(nonce: Nonce(Felt::ZERO)))),
     Nonce::default(),
     false
 )]
 #[case::should_not_skip_validation_non_invoke(
     Transaction::DeployAccount(
-        starknet_api::test_utils::deploy_account::executable_deploy_account_tx(
-            starknet_api::deploy_account_tx_args!(), Nonce::default()
-        )
+        executable_deploy_account_tx(deploy_account_tx_args!(), Nonce::default())
     ),
     Nonce::default(),
     false)]
 #[case::should_not_skip_validation_account_nonce_1(
-    Transaction::Invoke(starknet_api::test_utils::invoke::executable_invoke_tx(
-        starknet_api::invoke_tx_args!(
+    Transaction::Invoke(executable_invoke_tx(
+        invoke_tx_args!(
             nonce: Nonce(Felt::ONE),
             sender_address: TEST_SENDER_ADDRESS.into()
         )

crates/gateway/src/utils.rs

@@ -1,13 +1,11 @@
-use blockifier::transaction::account_transaction::AccountTransaction;
-use starknet_api::core::{ChainId, ContractAddress};
+use starknet_api::core::ChainId;
 use starknet_api::executable_transaction::{
     DeclareTransaction as ExecutableDeclareTransaction,
     DeployAccountTransaction as ExecutableDeployAccountTransaction,
     InvokeTransaction as ExecutableInvokeTransaction,
     Transaction as ExecutableTransaction,
 };
 use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction};
-use starknet_api::transaction::DeclareTransaction;
 use starknet_gateway_types::errors::GatewaySpecError;
 use tracing::{debug, error};
 
@@ -78,18 +76,3 @@ fn compile_contract_and_build_executable_declare_tx(
 
     Ok(executable_declare_tx)
 }
-
-// TODO(yael 9/5/54): Should be implemented as part of InternalTransaction in starknet-api
-pub fn get_sender_address(tx: &AccountTransaction) -> ContractAddress {
-    match tx {
-        AccountTransaction::Declare(tx) => match &tx.tx {
-            DeclareTransaction::V3(tx) => tx.sender_address,
-            _ => panic!("Unsupported transaction version"),
-        },
-        AccountTransaction::DeployAccount(tx) => tx.contract_address(),
-        AccountTransaction::Invoke(tx) => match &tx.tx() {
-            starknet_api::transaction::InvokeTransaction::V3(tx) => tx.sender_address,
-            _ => panic!("Unsupported transaction version"),
-        },
-    }
-}