From 4eff1f13777d5b04a895a8f0ecdfaf9787921d48 Mon Sep 17 00:00:00 2001 From: Aviv Greenburg Date: Mon, 2 Dec 2024 14:21:02 +0200 Subject: [PATCH] chore(blockifier): invoke() declare() deploy_account() change ret val to api_tx --- .../blockifier/transaction_executor_test.rs | 13 +-- .../src/concurrency/versioned_state_test.rs | 9 +- .../src/concurrency/worker_logic_test.rs | 3 +- crates/blockifier/src/test_utils/declare.rs | 8 +- .../src/test_utils/deploy_account.rs | 11 +- crates/blockifier/src/test_utils/invoke.rs | 11 +- .../src/test_utils/transfers_generator.rs | 6 +- .../transaction/account_transactions_test.rs | 6 +- .../blockifier/src/transaction/test_utils.rs | 23 ++-- .../src/transaction/transactions_test.rs | 102 +++++++++++------- .../native_blockifier/src/py_transaction.rs | 21 ++-- 11 files changed, 120 insertions(+), 93 deletions(-) diff --git a/crates/blockifier/src/blockifier/transaction_executor_test.rs b/crates/blockifier/src/blockifier/transaction_executor_test.rs index 3089818a87f..8deac77a253 100644 --- a/crates/blockifier/src/blockifier/transaction_executor_test.rs +++ b/crates/blockifier/src/blockifier/transaction_executor_test.rs @@ -29,6 +29,7 @@ use crate::test_utils::{ BALANCE, DEFAULT_STRK_L1_GAS_PRICE, }; +use crate::transaction::account_transaction::AccountTransaction; use crate::transaction::errors::TransactionExecutionError; use crate::transaction::test_utils::{ account_invoke_tx, @@ -120,7 +121,7 @@ fn test_declare( let declared_contract = FeatureContract::Empty(cairo_version); let state = test_state(&block_context.chain_info, BALANCE, &[(account_contract, 1)]); - let tx = declare_tx( + let declare_tx = declare_tx( declare_tx_args! { sender_address: account_contract.get_instance_address(0), class_hash: declared_contract.get_class_hash(), @@ -129,8 +130,8 @@ fn test_declare( resource_bounds: l1_resource_bounds(0_u8.into(), DEFAULT_STRK_L1_GAS_PRICE.into()), }, calculate_class_info_for_testing(declared_contract.get_class()), - ) - .into(); + ); + let tx = AccountTransaction { tx: declare_tx, only_query: false }.into(); tx_executor_test_body(state, block_context, tx, expected_bouncer_weights); } @@ -143,15 +144,15 @@ fn test_deploy_account( let account_contract = FeatureContract::AccountWithoutValidations(cairo_version); let state = test_state(&block_context.chain_info, BALANCE, &[(account_contract, 0)]); - let tx = deploy_account_tx( + let deploy_account_tx = deploy_account_tx( deploy_account_tx_args! { class_hash: account_contract.get_class_hash(), resource_bounds: l1_resource_bounds(0_u8.into(), DEFAULT_STRK_L1_GAS_PRICE.into()), version, }, &mut NonceManager::default(), - ) - .into(); + ); + let tx = AccountTransaction { tx: deploy_account_tx, only_query: false }.into(); let expected_bouncer_weights = BouncerWeights { state_diff_size: 3, message_segment_length: 0, diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index c817263ddc2..48a0a950651 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -44,6 +44,7 @@ use crate::test_utils::deploy_account::deploy_account_tx; use crate::test_utils::dict_state_reader::DictStateReader; use crate::test_utils::initial_test_state::test_state; use crate::test_utils::{CairoVersion, BALANCE, DEFAULT_STRK_L1_GAS_PRICE}; +use crate::transaction::account_transaction::AccountTransaction; use crate::transaction::objects::HasRelatedFeeType; use crate::transaction::test_utils::{default_all_resource_bounds, l1_resource_bounds}; use crate::transaction::transactions::ExecutableTransaction; @@ -225,7 +226,7 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { let mut state_2 = TransactionalState::create_transactional(&mut versioned_state_proxy_2); // Prepare transactions - let deploy_account_tx_1 = deploy_account_tx( + let tx = deploy_account_tx( deploy_account_tx_args! { class_hash: account_without_validation.get_class_hash(), resource_bounds: l1_resource_bounds( @@ -235,6 +236,7 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { }, &mut NonceManager::default(), ); + let deploy_account_tx_1 = AccountTransaction { tx, only_query: false }; let enforce_fee = deploy_account_tx_1.enforce_fee(); let class_hash = grindy_account.get_class_hash(); @@ -247,7 +249,10 @@ fn test_run_parallel_txs(default_all_resource_bounds: ValidResourceBounds) { constructor_calldata: constructor_calldata.clone(), }; let nonce_manager = &mut NonceManager::default(); - let delpoy_account_tx_2 = deploy_account_tx(deploy_tx_args, nonce_manager); + let delpoy_account_tx_2 = AccountTransaction { + tx: deploy_account_tx(deploy_tx_args, nonce_manager), + only_query: false, + }; let account_address = delpoy_account_tx_2.sender_address(); let tx_context = block_context.to_tx_context(&delpoy_account_tx_2); let fee_type = tx_context.tx_info.fee_type(); diff --git a/crates/blockifier/src/concurrency/worker_logic_test.rs b/crates/blockifier/src/concurrency/worker_logic_test.rs index 057d4bafab5..4370e43d170 100644 --- a/crates/blockifier/src/concurrency/worker_logic_test.rs +++ b/crates/blockifier/src/concurrency/worker_logic_test.rs @@ -549,7 +549,7 @@ fn test_deploy_before_declare( let test_class_hash = test_contract.get_class_hash(); let test_class_info = calculate_class_info_for_testing(test_contract.get_class()); let test_compiled_class_hash = test_contract.get_compiled_class_hash(); - let declare_tx = declare_tx( + let tx = declare_tx( declare_tx_args! { sender_address: account_address_0, resource_bounds: default_all_resource_bounds, @@ -561,6 +561,7 @@ fn test_deploy_before_declare( }, test_class_info.clone(), ); + let declare_tx = AccountTransaction { tx, only_query: false }; // Deploy test contract. let invoke_tx = account_invoke_tx(invoke_tx_args! { diff --git a/crates/blockifier/src/test_utils/declare.rs b/crates/blockifier/src/test_utils/declare.rs index 5ac24e5b49c..5afe538b74b 100644 --- a/crates/blockifier/src/test_utils/declare.rs +++ b/crates/blockifier/src/test_utils/declare.rs @@ -1,11 +1,11 @@ use starknet_api::contract_class::ClassInfo; -use starknet_api::executable_transaction::AccountTransaction as ExecutableTransaction; +use starknet_api::executable_transaction::AccountTransaction; use starknet_api::test_utils::declare::{executable_declare_tx, DeclareTxArgs}; -use crate::transaction::account_transaction::AccountTransaction; - +// TODO(AvivG): remove this func & file. pub fn declare_tx(declare_tx_args: DeclareTxArgs, class_info: ClassInfo) -> AccountTransaction { + // TODO(AvivG): see into making 'executable_declare_tx' ret type AccountTransaction. let declare_tx = executable_declare_tx(declare_tx_args, class_info); - AccountTransaction { tx: ExecutableTransaction::Declare(declare_tx), only_query: false } + AccountTransaction::Declare(declare_tx) } diff --git a/crates/blockifier/src/test_utils/deploy_account.rs b/crates/blockifier/src/test_utils/deploy_account.rs index 1e40d4e188e..711759ae7fc 100644 --- a/crates/blockifier/src/test_utils/deploy_account.rs +++ b/crates/blockifier/src/test_utils/deploy_account.rs @@ -1,17 +1,14 @@ -use starknet_api::executable_transaction::AccountTransaction as ExecutableTransaction; +use starknet_api::executable_transaction::AccountTransaction; use starknet_api::test_utils::deploy_account::{executable_deploy_account_tx, DeployAccountTxArgs}; use starknet_api::test_utils::NonceManager; -use crate::transaction::account_transaction::AccountTransaction; - +// TODO(AvivG): remove this func & file. pub fn deploy_account_tx( deploy_tx_args: DeployAccountTxArgs, nonce_manager: &mut NonceManager, ) -> AccountTransaction { + // TODO(AvivG): see into making 'executable_deploy_account_tx' ret type AccountTransaction. let deploy_account_tx = executable_deploy_account_tx(deploy_tx_args, nonce_manager); - AccountTransaction { - tx: ExecutableTransaction::DeployAccount(deploy_account_tx), - only_query: false, - } + AccountTransaction::DeployAccount(deploy_account_tx) } diff --git a/crates/blockifier/src/test_utils/invoke.rs b/crates/blockifier/src/test_utils/invoke.rs index b4e4b3a530e..21a0858d73b 100644 --- a/crates/blockifier/src/test_utils/invoke.rs +++ b/crates/blockifier/src/test_utils/invoke.rs @@ -1,11 +1,8 @@ -use starknet_api::executable_transaction::AccountTransaction as ExecutableTransaction; +use starknet_api::executable_transaction::AccountTransaction; use starknet_api::test_utils::invoke::{executable_invoke_tx, InvokeTxArgs}; -use crate::transaction::account_transaction::AccountTransaction; - +// TODO(AvivG): remove this func & file. pub fn invoke_tx(invoke_args: InvokeTxArgs) -> AccountTransaction { - let only_query = invoke_args.only_query; - let invoke_tx = ExecutableTransaction::Invoke(executable_invoke_tx(invoke_args)); - - AccountTransaction { tx: invoke_tx, only_query } + // TODO(AvivG): see into making 'executable_invoke_tx' ret type AccountTransaction. + AccountTransaction::Invoke(executable_invoke_tx(invoke_args)) } diff --git a/crates/blockifier/src/test_utils/transfers_generator.rs b/crates/blockifier/src/test_utils/transfers_generator.rs index 0915dc82f1b..213215ca104 100644 --- a/crates/blockifier/src/test_utils/transfers_generator.rs +++ b/crates/blockifier/src/test_utils/transfers_generator.rs @@ -2,6 +2,7 @@ use rand::rngs::StdRng; use rand::{Rng, SeedableRng}; use starknet_api::abi::abi_utils::selector_from_name; use starknet_api::core::ContractAddress; +use starknet_api::executable_transaction::AccountTransaction as ApiExecutableTransaction; use starknet_api::test_utils::NonceManager; use starknet_api::transaction::constants::TRANSFER_ENTRY_POINT_NAME; use starknet_api::transaction::fields::Fee; @@ -143,7 +144,8 @@ impl TransfersGenerator { let recipient_address = self.get_next_recipient(); self.sender_index = (self.sender_index + 1) % self.account_addresses.len(); - let account_tx = self.generate_transfer(sender_address, recipient_address); + let tx = self.generate_transfer(sender_address, recipient_address); + let account_tx = AccountTransaction { tx, only_query: false }; txs.push(Transaction::Account(account_tx)); } let results = self.executor.execute_txs(&txs); @@ -159,7 +161,7 @@ impl TransfersGenerator { &mut self, sender_address: ContractAddress, recipient_address: ContractAddress, - ) -> AccountTransaction { + ) -> ApiExecutableTransaction { let nonce = self.nonce_manager.next(sender_address); let entry_point_selector = selector_from_name(TRANSFER_ENTRY_POINT_NAME); diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index b7eca58eaf5..d070ca9fa37 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -190,7 +190,7 @@ fn test_fee_enforcement( ) { let account = FeatureContract::AccountWithoutValidations(CairoVersion::Cairo0); let state = &mut test_state(&block_context.chain_info, BALANCE, &[(account, 1)]); - let deploy_account_tx = deploy_account_tx( + let tx = deploy_account_tx( deploy_account_tx_args! { class_hash: account.get_class_hash(), max_fee: Fee(if zero_bounds { 0 } else { MAX_FEE.0 }), @@ -212,6 +212,7 @@ fn test_fee_enforcement( }, &mut NonceManager::default(), ); + let deploy_account_tx = AccountTransaction { tx, only_query: false }; let enforce_fee = deploy_account_tx.enforce_fee(); assert_ne!(zero_bounds, enforce_fee); @@ -451,7 +452,7 @@ fn test_max_fee_limit_validate( let class_info = calculate_class_info_for_testing(grindy_validate_account.get_class()); // Declare the grindy-validation account. - let account_tx = declare_tx( + let tx = declare_tx( declare_tx_args! { class_hash: grindy_class_hash, sender_address: account_address, @@ -460,6 +461,7 @@ fn test_max_fee_limit_validate( }, class_info, ); + let account_tx = AccountTransaction { tx, only_query: false }; account_tx.execute(&mut state, &block_context, true, true).unwrap(); // Deploy grindy account with a lot of grind in the constructor. diff --git a/crates/blockifier/src/transaction/test_utils.rs b/crates/blockifier/src/transaction/test_utils.rs index 36757124a48..68a2b989362 100644 --- a/crates/blockifier/src/transaction/test_utils.rs +++ b/crates/blockifier/src/transaction/test_utils.rs @@ -113,7 +113,10 @@ pub fn deploy_and_fund_account( deploy_tx_args: DeployAccountTxArgs, ) -> (AccountTransaction, ContractAddress) { // Deploy an account contract. - let deploy_account_tx = deploy_account_tx(deploy_tx_args, nonce_manager); + let deploy_account_tx = AccountTransaction { + tx: deploy_account_tx(deploy_tx_args, nonce_manager), + only_query: false, + }; let account_address = deploy_account_tx.sender_address(); // Update the balance of the about-to-be deployed account contract in the erc20 contract, so it @@ -235,7 +238,7 @@ pub fn create_account_tx_for_validate_test( }; let class_hash = declared_contract.get_class_hash(); let class_info = calculate_class_info_for_testing(declared_contract.get_class()); - declare_tx( + let tx = declare_tx( declare_tx_args! { max_fee, resource_bounds, @@ -247,7 +250,8 @@ pub fn create_account_tx_for_validate_test( compiled_class_hash: declared_contract.get_compiled_class_hash(), }, class_info, - ) + ); + AccountTransaction { tx, only_query: false } } TransactionType::DeployAccount => { // We do not use the sender address here because the transaction generates the actual @@ -256,7 +260,7 @@ pub fn create_account_tx_for_validate_test( true => constants::FELT_TRUE, false => constants::FELT_FALSE, })]; - deploy_account_tx( + let tx = deploy_account_tx( deploy_account_tx_args! { max_fee, resource_bounds, @@ -267,11 +271,12 @@ pub fn create_account_tx_for_validate_test( constructor_calldata, }, nonce_manager, - ) + ); + AccountTransaction { tx, only_query: false } } TransactionType::InvokeFunction => { let execute_calldata = create_calldata(sender_address, "foo", &[]); - invoke_tx(invoke_tx_args! { + let tx = invoke_tx(invoke_tx_args! { max_fee, resource_bounds, signature, @@ -279,14 +284,16 @@ pub fn create_account_tx_for_validate_test( calldata: execute_calldata, version: tx_version, nonce: nonce_manager.next(sender_address), - }) + }); + AccountTransaction { tx, only_query: false } } _ => panic!("{tx_type:?} is not an account transaction."), } } pub fn account_invoke_tx(invoke_args: InvokeTxArgs) -> AccountTransaction { - invoke_tx(invoke_args) + let only_query = invoke_args.only_query; + AccountTransaction { tx: invoke_tx(invoke_args), only_query } } pub fn run_invoke_tx( diff --git a/crates/blockifier/src/transaction/transactions_test.rs b/crates/blockifier/src/transaction/transactions_test.rs index 0f842f8335a..f6fb7c0388b 100644 --- a/crates/blockifier/src/transaction/transactions_test.rs +++ b/crates/blockifier/src/transaction/transactions_test.rs @@ -449,11 +449,12 @@ fn test_invoke_tx( let test_contract_address = test_contract.get_instance_address(0); let account_contract_address = account_contract.get_instance_address(0); let calldata = create_trivial_calldata(test_contract_address); - let invoke_tx = invoke_tx(invoke_tx_args! { + let tx = invoke_tx(invoke_tx_args! { sender_address: account_contract_address, calldata: Calldata(Arc::clone(&calldata.0)), resource_bounds, }); + let invoke_tx = AccountTransaction { tx, only_query: false }; // Extract invoke transaction fields for testing, as it is consumed when creating an account // transaction. @@ -967,13 +968,16 @@ fn test_max_fee_exceeds_balance( )}; // Deploy. - let invalid_tx = deploy_account_tx( - deploy_account_tx_args! { - resource_bounds, - class_hash: test_contract.get_class_hash() - }, - &mut NonceManager::default(), - ); + let invalid_tx = AccountTransaction { + tx: deploy_account_tx( + deploy_account_tx_args! { + resource_bounds, + class_hash: test_contract.get_class_hash() + }, + &mut NonceManager::default(), + ), + only_query: false, + }; assert_resource_bounds_exceed_balance_failure(state, block_context, invalid_tx); // V1 Invoke. @@ -997,15 +1001,18 @@ fn test_max_fee_exceeds_balance( // Declare. let contract_to_declare = FeatureContract::Empty(CairoVersion::Cairo1); let class_info = calculate_class_info_for_testing(contract_to_declare.get_class()); - let invalid_tx = declare_tx( - declare_tx_args! { - class_hash: contract_to_declare.get_class_hash(), - compiled_class_hash: contract_to_declare.get_compiled_class_hash(), - sender_address, - resource_bounds: $invalid_resource_bounds, - }, - class_info, - ); + let invalid_tx = AccountTransaction { + tx: declare_tx( + declare_tx_args! { + class_hash: contract_to_declare.get_class_hash(), + compiled_class_hash: contract_to_declare.get_compiled_class_hash(), + sender_address, + resource_bounds: $invalid_resource_bounds, + }, + class_info, + ), + only_query: false, + }; assert_resource_bounds_exceed_balance_failure(state, block_context, invalid_tx); }; } @@ -1508,7 +1515,7 @@ fn test_declare_tx( None, ExecutionSummary::default(), ); - let account_tx = declare_tx( + let tx = declare_tx( declare_tx_args! { max_fee: MAX_FEE, sender_address, @@ -1520,6 +1527,7 @@ fn test_declare_tx( }, class_info.clone(), ); + let account_tx = AccountTransaction { tx, only_query: false }; // Check state before transaction application. assert_matches!( @@ -1629,7 +1637,7 @@ fn test_declare_tx( assert_eq!(contract_class_from_state, class_info.contract_class().try_into().unwrap()); // Checks that redeclaring the same contract fails. - let account_tx2 = declare_tx( + let tx2 = declare_tx( declare_tx_args! { max_fee: MAX_FEE, sender_address, @@ -1641,6 +1649,7 @@ fn test_declare_tx( }, class_info.clone(), ); + let account_tx2 = AccountTransaction { tx: tx2, only_query: false }; let result = account_tx2.execute(state, block_context, true, true); assert_matches!( result.unwrap_err(), @@ -1663,7 +1672,7 @@ fn test_declare_tx_v0(default_l1_resource_bounds: ValidResourceBounds) { let sender_address = account.get_instance_address(0); let mut nonce_manager = NonceManager::default(); - let account_tx = declare_tx( + let tx = declare_tx( declare_tx_args! { max_fee: Fee(0), sender_address, @@ -1675,6 +1684,7 @@ fn test_declare_tx_v0(default_l1_resource_bounds: ValidResourceBounds) { }, class_info.clone(), ); + let account_tx = AccountTransaction { tx, only_query: false }; let actual_execution_info = account_tx.execute(state, block_context, false, true).unwrap(); // fee not charged for declare v0. @@ -1695,13 +1705,16 @@ fn test_deploy_account_tx( let account = FeatureContract::AccountWithoutValidations(cairo_version); let account_class_hash = account.get_class_hash(); let state = &mut test_state(chain_info, BALANCE, &[(account, 1)]); - let deploy_account = deploy_account_tx( - deploy_account_tx_args! { - resource_bounds: default_all_resource_bounds, - class_hash: account_class_hash - }, - &mut nonce_manager, - ); + let deploy_account = AccountTransaction { + tx: deploy_account_tx( + deploy_account_tx_args! { + resource_bounds: default_all_resource_bounds, + class_hash: account_class_hash + }, + &mut nonce_manager, + ), + only_query: false, + }; // Extract deploy account transaction fields for testing, as it is consumed when creating an // account transaction. @@ -1852,13 +1865,16 @@ fn test_deploy_account_tx( // Negative flow. // Deploy to an existing address. - let deploy_account = deploy_account_tx( - deploy_account_tx_args! { - resource_bounds: default_all_resource_bounds, - class_hash: account_class_hash - }, - &mut nonce_manager, - ); + let deploy_account = AccountTransaction { + tx: deploy_account_tx( + deploy_account_tx_args! { + resource_bounds: default_all_resource_bounds, + class_hash: account_class_hash + }, + &mut nonce_manager, + ), + only_query: false, + }; let error = deploy_account.execute(state, block_context, true, true).unwrap_err(); assert_matches!( error, @@ -1883,12 +1899,15 @@ fn test_fail_deploy_account_undeclared_class_hash( let state = &mut test_state(chain_info, BALANCE, &[]); let mut nonce_manager = NonceManager::default(); let undeclared_hash = class_hash!("0xdeadbeef"); - let deploy_account = deploy_account_tx( - deploy_account_tx_args! { - resource_bounds: default_all_resource_bounds, class_hash: undeclared_hash - }, - &mut nonce_manager, - ); + let deploy_account = AccountTransaction { + tx: deploy_account_tx( + deploy_account_tx_args! { + resource_bounds: default_all_resource_bounds, class_hash: undeclared_hash + }, + &mut nonce_manager, + ), + only_query: false, + }; let tx_context = block_context.to_tx_context(&deploy_account); let fee_type = tx_context.tx_info.fee_type(); @@ -2227,12 +2246,13 @@ fn test_only_query_flag( .concat() .into(), ); - let invoke_tx = crate::test_utils::invoke::invoke_tx(invoke_tx_args! { + let tx = crate::test_utils::invoke::invoke_tx(invoke_tx_args! { calldata: execute_calldata, resource_bounds: default_all_resource_bounds, sender_address, only_query, }); + let invoke_tx = AccountTransaction { tx, only_query }; let tx_execution_info = invoke_tx.execute(state, block_context, true, true).unwrap(); assert_eq!(tx_execution_info.revert_error, None); diff --git a/crates/native_blockifier/src/py_transaction.rs b/crates/native_blockifier/src/py_transaction.rs index d012c0fa164..1b6782734ac 100644 --- a/crates/native_blockifier/src/py_transaction.rs +++ b/crates/native_blockifier/src/py_transaction.rs @@ -138,22 +138,17 @@ pub fn py_tx( TransactionType::Declare => { let non_optional_py_class_info: PyClassInfo = optional_py_class_info .expect("A class info must be passed in a Declare transaction."); - AccountTransaction { - tx: ExecutableTransaction::Declare(py_declare(tx, non_optional_py_class_info)?), - only_query: false, - } - .into() + let tx = ExecutableTransaction::Declare(py_declare(tx, non_optional_py_class_info)?); + AccountTransaction { tx, only_query: false }.into() } - TransactionType::DeployAccount => AccountTransaction { - tx: ExecutableTransaction::DeployAccount(py_deploy_account(tx)?), - only_query: false, + TransactionType::DeployAccount => { + let tx = ExecutableTransaction::DeployAccount(py_deploy_account(tx)?); + AccountTransaction { tx, only_query: false }.into() } - .into(), - TransactionType::InvokeFunction => AccountTransaction { - tx: ExecutableTransaction::Invoke(py_invoke_function(tx)?), - only_query: false, + TransactionType::InvokeFunction => { + let tx = ExecutableTransaction::Invoke(py_invoke_function(tx)?); + AccountTransaction { tx, only_query: false }.into() } - .into(), TransactionType::L1Handler => py_l1_handler(tx)?.into(), }) }