From 54388cf451613815b1439b9e72b5792e52385285 Mon Sep 17 00:00:00 2001 From: YaelD <70628564+Yael-Starkware@users.noreply.github.com> Date: Mon, 5 Aug 2024 10:31:58 +0300 Subject: [PATCH] chore: remove duplicate calculation of sender_address and tx_hash in the gateway (#151) --- crates/gateway/src/errors.rs | 2 - crates/gateway/src/gateway.rs | 14 +++--- .../src/stateful_transaction_validator.rs | 18 ++++++-- .../stateful_transaction_validator_test.rs | 44 ++++++++++--------- crates/gateway/src/utils.rs | 17 ++++++- 5 files changed, 63 insertions(+), 32 deletions(-) diff --git a/crates/gateway/src/errors.rs b/crates/gateway/src/errors.rs index 2801b3dce7..a4ffe6af03 100644 --- a/crates/gateway/src/errors.rs +++ b/crates/gateway/src/errors.rs @@ -55,8 +55,6 @@ pub enum GatewayError { #[error("Error sending message: {0}")] MessageSendError(String), #[error(transparent)] - StarknetApiError(#[from] StarknetApiError), - #[error(transparent)] StatefulTransactionValidatorError(#[from] StatefulTransactionValidatorError), #[error(transparent)] StatelessTransactionValidatorError(#[from] StatelessTransactionValidatorError), diff --git a/crates/gateway/src/gateway.rs b/crates/gateway/src/gateway.rs index 70228f7251..a7e1af4899 100644 --- a/crates/gateway/src/gateway.rs +++ b/crates/gateway/src/gateway.rs @@ -10,7 +10,7 @@ use starknet_api::rpc_transaction::RpcTransaction; use starknet_api::transaction::TransactionHash; use starknet_mempool_infra::component_runner::{ComponentStartError, ComponentStarter}; use starknet_mempool_types::communication::SharedMempoolClient; -use starknet_mempool_types::mempool_types::{Account, MempoolInput}; +use starknet_mempool_types::mempool_types::{Account, AccountState, MempoolInput}; use tracing::{info, instrument}; use crate::compilation::GatewayCompiler; @@ -134,13 +134,17 @@ fn process_tx( }; let validator = stateful_tx_validator.instantiate_validator(state_reader_factory)?; - let tx_hash = stateful_tx_validator.run_validate(&tx, optional_class_info, validator)?; + // TODO(Yael 31/7/24): refactor after IntrnalTransaction is ready, delete validate_info and + // compute all the info outside of run_validate. + let validate_info = stateful_tx_validator.run_validate(&tx, optional_class_info, validator)?; // TODO(Arni): Add the Sierra and the Casm to the mempool input. - let sender_address = tx.calculate_sender_address()?; Ok(MempoolInput { - tx: external_tx_to_thin_tx(&tx, tx_hash, sender_address), - account: Account { sender_address, ..Default::default() }, + tx: external_tx_to_thin_tx(&tx, validate_info.tx_hash, validate_info.sender_address), + account: Account { + sender_address: validate_info.sender_address, + state: AccountState { nonce: validate_info.account_nonce }, + }, }) } diff --git a/crates/gateway/src/stateful_transaction_validator.rs b/crates/gateway/src/stateful_transaction_validator.rs index 201de30fbf..6e140f1cf5 100644 --- a/crates/gateway/src/stateful_transaction_validator.rs +++ b/crates/gateway/src/stateful_transaction_validator.rs @@ -19,7 +19,7 @@ use starknet_types_core::felt::Felt; use crate::config::StatefulTransactionValidatorConfig; use crate::errors::StatefulTransactionValidatorResult; use crate::state_reader::{MempoolStateReader, StateReaderFactory}; -use crate::utils::{external_tx_to_account_tx, get_tx_hash}; +use crate::utils::{external_tx_to_account_tx, get_sender_address, get_tx_hash}; #[cfg(test)] #[path = "stateful_transaction_validator_test.rs"] @@ -69,17 +69,18 @@ impl StatefulTransactionValidator { external_tx: &RpcTransaction, optional_class_info: Option, mut validator: V, - ) -> StatefulTransactionValidatorResult { + ) -> StatefulTransactionValidatorResult { let account_tx = external_tx_to_account_tx( external_tx, optional_class_info, &self.config.chain_info.chain_id, )?; let tx_hash = get_tx_hash(&account_tx); - let account_nonce = validator.get_nonce(external_tx.calculate_sender_address()?)?; + let sender_address = get_sender_address(&account_tx); + let account_nonce = validator.get_nonce(sender_address)?; let skip_validate = skip_stateful_validations(external_tx, account_nonce); validator.validate(account_tx, skip_validate)?; - Ok(tx_hash) + Ok(ValidateInfo { tx_hash, sender_address, account_nonce }) } pub fn instantiate_validator( @@ -131,3 +132,12 @@ pub fn get_latest_block_info( let state_reader = state_reader_factory.get_state_reader_from_latest_block(); Ok(state_reader.get_block_info()?) } + +/// Holds members created by the stateful transaction validator, needed for +/// [`MempoolInput`](starknet_mempool_types::mempool_types::MempoolInput). +#[derive(Debug)] +pub struct ValidateInfo { + pub tx_hash: TransactionHash, + pub sender_address: ContractAddress, + pub account_nonce: Nonce, +} diff --git a/crates/gateway/src/stateful_transaction_validator_test.rs b/crates/gateway/src/stateful_transaction_validator_test.rs index d06e9bd7c1..d510a6424e 100644 --- a/crates/gateway/src/stateful_transaction_validator_test.rs +++ b/crates/gateway/src/stateful_transaction_validator_test.rs @@ -1,4 +1,4 @@ -use blockifier::blockifier::stateful_validator::StatefulValidatorError; +use blockifier::blockifier::stateful_validator::{StatefulValidatorError, StatefulValidatorResult}; use blockifier::context::BlockContext; use blockifier::test_utils::CairoVersion; use blockifier::transaction::errors::{TransactionFeeError, TransactionPreValidationError}; @@ -31,6 +31,20 @@ use crate::stateful_transaction_validator::{ StatefulTransactionValidator, }; +pub const STATEFUL_VALIDATOR_FEE_ERROR: StatefulValidatorError = + StatefulValidatorError::TransactionPreValidationError( + TransactionPreValidationError::TransactionFeeError( + TransactionFeeError::L1GasBoundsExceedBalance { + max_amount: VALID_L1_GAS_MAX_AMOUNT, + max_price: VALID_L1_GAS_MAX_PRICE_PER_UNIT, + balance: BigUint::ZERO, + }, + ), + ); + +pub const STATEFUL_TRANSACTION_VALIDATOR_FEE_ERROR: StatefulTransactionValidatorError = + StatefulTransactionValidatorError::StatefulValidatorError(STATEFUL_VALIDATOR_FEE_ERROR); + #[fixture] fn block_context() -> BlockContext { BlockContext::create_for_testing() @@ -51,26 +65,19 @@ fn stateful_validator(block_context: BlockContext) -> StatefulTransactionValidat #[rstest] #[case::valid_tx( invoke_tx(CairoVersion::Cairo1), + Ok(()), Ok(TransactionHash(felt!( "0x152b8dd0c30e95fa3a4ee7a9398fcfc46fb00c048b4fdcfa9958c64d65899b8" ))) )] #[case::invalid_tx( invoke_tx(CairoVersion::Cairo1), - Err(StatefulTransactionValidatorError::StatefulValidatorError( - StatefulValidatorError::TransactionPreValidationError( - TransactionPreValidationError::TransactionFeeError( - TransactionFeeError::L1GasBoundsExceedBalance { - max_amount: VALID_L1_GAS_MAX_AMOUNT, - max_price: VALID_L1_GAS_MAX_PRICE_PER_UNIT, - balance: BigUint::ZERO, - } - ) - ) - )) + Err(STATEFUL_VALIDATOR_FEE_ERROR), + Err(STATEFUL_TRANSACTION_VALIDATOR_FEE_ERROR) )] fn test_stateful_tx_validator( #[case] external_tx: RpcTransaction, + #[case] expected_blockifier_response: StatefulValidatorResult<()>, #[case] expected_result: StatefulTransactionValidatorResult, stateful_validator: StatefulTransactionValidator, ) { @@ -83,18 +90,15 @@ fn test_stateful_tx_validator( _ => None, }; - let expected_result_msg = format!("{:?}", expected_result); - let mut mock_validator = MockStatefulTransactionValidatorTrait::new(); - mock_validator.expect_validate().return_once(|_, _| match expected_result { - Ok(_) => Ok(()), - Err(StatefulTransactionValidatorError::StatefulValidatorError(err)) => Err(err), - _ => panic!("Expecting StatefulTransactionValidatorError::StatefulValidatorError"), - }); + mock_validator.expect_validate().return_once(|_, _| expected_blockifier_response); mock_validator.expect_get_nonce().returning(|_| Ok(Nonce(Felt::ZERO))); let result = stateful_validator.run_validate(&external_tx, optional_class_info, mock_validator); - assert_eq!(format!("{:?}", result), expected_result_msg); + match expected_result { + Ok(expected_result) => assert_eq!(result.unwrap().tx_hash, expected_result), + Err(_) => assert_eq!(format!("{:?}", result), format!("{:?}", expected_result)), + } } #[test] diff --git a/crates/gateway/src/utils.rs b/crates/gateway/src/utils.rs index 7ad61d61fc..2823f00098 100644 --- a/crates/gateway/src/utils.rs +++ b/crates/gateway/src/utils.rs @@ -115,7 +115,7 @@ pub fn external_tx_to_account_tx( } } -// TODO(yael 9/5/54): Remove once we we transition to InternalTransaction +// TODO(yael 9/5/54): Should be implemented as part of InternalTransaction in starknet-api pub fn get_tx_hash(tx: &AccountTransaction) -> TransactionHash { match tx { AccountTransaction::Declare(tx) => tx.tx_hash, @@ -123,3 +123,18 @@ pub fn get_tx_hash(tx: &AccountTransaction) -> TransactionHash { AccountTransaction::Invoke(tx) => tx.tx_hash, } } + +// 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 { + InvokeTransaction::V3(tx) => tx.sender_address, + _ => panic!("Unsupported transaction version"), + }, + } +}