From 176981bd386340fb4e7eda8743394fa709f1a16d Mon Sep 17 00:00:00 2001 From: Arni Hod Date: Mon, 9 Sep 2024 14:31:24 +0300 Subject: [PATCH] refactor: use the proper executable tx as mempool input in gateway --- crates/gateway/src/gateway.rs | 8 +- crates/gateway/src/gateway_test.rs | 185 ++---------------- .../src/stateful_transaction_validator.rs | 7 +- .../stateful_transaction_validator_test.rs | 4 +- crates/gateway/src/utils.rs | 19 +- 5 files changed, 23 insertions(+), 200 deletions(-) diff --git a/crates/gateway/src/gateway.rs b/crates/gateway/src/gateway.rs index 63b658b3cc5..3ff0b83e64f 100644 --- a/crates/gateway/src/gateway.rs +++ b/crates/gateway/src/gateway.rs @@ -130,8 +130,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, @@ -152,11 +150,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 }, diff --git a/crates/gateway/src/gateway_test.rs b/crates/gateway/src/gateway_test.rs index ee2d8035bae..89a43e4d034 100644 --- a/crates/gateway/src/gateway_test.rs +++ b/crates/gateway/src/gateway_test.rs @@ -5,48 +5,19 @@ use axum::body::{Bytes, HttpBody}; use axum::extract::State; use axum::http::StatusCode; use axum::response::{IntoResponse, Response}; -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_mempool_types::communication::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::{GatewaySpecError, StatefulTransactionValidatorResult}; +use crate::errors::GatewaySpecError; use crate::gateway::{add_tx, AppState, SharedMempoolClient}; use crate::state_reader_test_utils::{local_test_state_reader_factory, TestStateReaderFactory}; use crate::stateful_transaction_validator::StatefulTransactionValidator; @@ -86,30 +57,28 @@ fn create_tx() -> (RpcTransaction, SenderAddress) { #[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(MempoolInput { - // TODO(Arni): Use external_to_executable_tx instead of `create_executable_tx`. Consider - // creating a `convertor for testing` that does not do the compilation. - tx: create_executable_tx( - sender_address, - tx_hash, - *tx.tip(), - *tx.nonce(), - ValidResourceBounds::AllResources(tx.resource_bounds().clone()), - ), - account: Account { sender_address, state: AccountState { nonce: *tx.nonce() } }, + tx: executable_tx, + account: Account { sender_address, state: AccountState { nonce: *rpc_tx.nonce() } }, })) .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 = add_tx(State(app_state), tx.into()).await.into_response(); + let response = add_tx(State(app_state), rpc_tx.into()).await.into_response(); let status_code = response.status(); let response_bytes = &to_bytes(response).await; @@ -140,125 +109,3 @@ async fn test_compiled_class_hash_mismatch() { let err = add_tx(State(app_state), tx.into()).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, - chain_id: &ChainId, -) -> StatefulTransactionValidatorResult { - 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)) - } - } -} diff --git a/crates/gateway/src/stateful_transaction_validator.rs b/crates/gateway/src/stateful_transaction_validator.rs index 7ee942bcd5e..70e491cec97 100644 --- a/crates/gateway/src/stateful_transaction_validator.rs +++ b/crates/gateway/src/stateful_transaction_validator.rs @@ -22,7 +22,6 @@ use tracing::error; use crate::config::StatefulTransactionValidatorConfig; use crate::errors::{GatewaySpecError, StatefulTransactionValidatorResult}; use crate::state_reader::{MempoolStateReader, StateReaderFactory}; -use crate::utils::get_sender_address; #[cfg(test)] #[path = "stateful_transaction_validator_test.rs"] @@ -74,6 +73,9 @@ impl StatefulTransactionValidator { executable_tx: &ExecutableTransaction, mut validator: V, ) -> StatefulTransactionValidatorResult { + 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(), @@ -82,8 +84,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() } @@ -148,6 +148,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)] diff --git a/crates/gateway/src/stateful_transaction_validator_test.rs b/crates/gateway/src/stateful_transaction_validator_test.rs index 9cf02d1b242..185741ffce9 100644 --- a/crates/gateway/src/stateful_transaction_validator_test.rs +++ b/crates/gateway/src/stateful_transaction_validator_test.rs @@ -64,9 +64,7 @@ fn stateful_validator(block_context: BlockContext) -> StatefulTransactionValidat #[case::valid_tx( executable_invoke_tx(CairoVersion::Cairo1), Ok(ValidateInfo{ - tx_hash: TransactionHash(felt!( - "0x3b93426272b6e281bc9bde29b91a9fb100c2f9689388c62360b2be2f4e7b493" - )), + tx_hash: TransactionHash::default(), sender_address: contract_address!("0xc0020000"), account_nonce: Nonce::default() }) diff --git a/crates/gateway/src/utils.rs b/crates/gateway/src/utils.rs index 06316f2959b..449c24d17a3 100644 --- a/crates/gateway/src/utils.rs +++ b/crates/gateway/src/utils.rs @@ -1,5 +1,4 @@ -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, @@ -7,7 +6,6 @@ use starknet_api::executable_transaction::{ Transaction as ExecutableTransaction, }; use starknet_api::rpc_transaction::{RpcDeclareTransaction, RpcTransaction}; -use starknet_api::transaction::DeclareTransaction; use tracing::{debug, error}; use crate::compilation::GatewayCompiler; @@ -77,18 +75,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"), - }, - } -}