From f7ff378b6ff0ffdcfd3d580adc47a774b5bd76f4 Mon Sep 17 00:00:00 2001 From: nimrod-starkware <143319383+nimrod-starkware@users.noreply.github.com> Date: Wed, 28 Aug 2024 16:57:33 +0300 Subject: [PATCH] refactor(fee): transaction info creator may fail (#506) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change is [Reviewable](https://reviewable.io/reviews/starkware-libs/sequencer/506) --- .../src/blockifier/stateful_validator.rs | 8 ++- .../src/concurrency/versioned_state_test.rs | 2 +- crates/blockifier/src/context.rs | 2 +- .../src/transaction/account_transaction.rs | 4 +- .../transaction/account_transactions_test.rs | 4 +- crates/blockifier/src/transaction/errors.rs | 9 ++++ crates/blockifier/src/transaction/objects.rs | 3 +- .../src/transaction/post_execution_test.rs | 2 +- .../src/transaction/transaction_execution.rs | 4 +- .../src/transaction/transactions.rs | 52 +++++++++---------- crates/native_blockifier/src/py_validator.rs | 2 +- 11 files changed, 55 insertions(+), 37 deletions(-) diff --git a/crates/blockifier/src/blockifier/stateful_validator.rs b/crates/blockifier/src/blockifier/stateful_validator.rs index 3c6a8606dd..c93bbef268 100644 --- a/crates/blockifier/src/blockifier/stateful_validator.rs +++ b/crates/blockifier/src/blockifier/stateful_validator.rs @@ -18,7 +18,11 @@ use crate::state::cached_state::CachedState; use crate::state::errors::StateError; use crate::state::state_api::StateReader; use crate::transaction::account_transaction::AccountTransaction; -use crate::transaction::errors::{TransactionExecutionError, TransactionPreValidationError}; +use crate::transaction::errors::{ + TransactionExecutionError, + TransactionInfoCreationError, + TransactionPreValidationError, +}; use crate::transaction::transaction_execution::Transaction; use crate::transaction::transactions::ValidatableTransaction; @@ -36,6 +40,8 @@ pub enum StatefulValidatorError { TransactionExecutorError(#[from] TransactionExecutorError), #[error(transparent)] TransactionPreValidationError(#[from] TransactionPreValidationError), + #[error(transparent)] + TransactionCreationError(#[from] TransactionInfoCreationError), } pub type StatefulValidatorResult = Result; diff --git a/crates/blockifier/src/concurrency/versioned_state_test.rs b/crates/blockifier/src/concurrency/versioned_state_test.rs index f3e0ff0bf3..a913f60484 100644 --- a/crates/blockifier/src/concurrency/versioned_state_test.rs +++ b/crates/blockifier/src/concurrency/versioned_state_test.rs @@ -233,7 +233,7 @@ fn test_run_parallel_txs(max_resource_bounds: DeprecatedResourceBoundsMapping) { &mut NonceManager::default(), ); let account_tx_1 = AccountTransaction::DeployAccount(deploy_account_tx_1); - let enforce_fee = account_tx_1.create_tx_info().enforce_fee(); + let enforce_fee = account_tx_1.create_tx_info().unwrap().enforce_fee(); let class_hash = grindy_account.get_class_hash(); let ctor_storage_arg = felt!(1_u8); diff --git a/crates/blockifier/src/context.rs b/crates/blockifier/src/context.rs index 13b356ea6d..7f7864fffb 100644 --- a/crates/blockifier/src/context.rs +++ b/crates/blockifier/src/context.rs @@ -68,7 +68,7 @@ impl BlockContext { ) -> TransactionContext { TransactionContext { block_context: self.clone(), - tx_info: tx_info_creator.create_tx_info(), + tx_info: tx_info_creator.create_tx_info().expect("todo"), } } } diff --git a/crates/blockifier/src/transaction/account_transaction.rs b/crates/blockifier/src/transaction/account_transaction.rs index 0dbfe9dfc1..204659db3a 100644 --- a/crates/blockifier/src/transaction/account_transaction.rs +++ b/crates/blockifier/src/transaction/account_transaction.rs @@ -33,6 +33,7 @@ use crate::transaction::constants; use crate::transaction::errors::{ TransactionExecutionError, TransactionFeeError, + TransactionInfoCreationError, TransactionPreValidationError, }; use crate::transaction::objects::{ @@ -737,7 +738,8 @@ impl ExecutableTransaction for AccountTransaction { } impl TransactionInfoCreator for AccountTransaction { - fn create_tx_info(&self) -> TransactionInfo { + // TODO(Nimrod): This function should return `TransactionInfo` without a result. + fn create_tx_info(&self) -> Result { match self { Self::Declare(tx) => tx.create_tx_info(), Self::DeployAccount(tx) => tx.create_tx_info(), diff --git a/crates/blockifier/src/transaction/account_transactions_test.rs b/crates/blockifier/src/transaction/account_transactions_test.rs index 6a684d21f8..7652e251bd 100644 --- a/crates/blockifier/src/transaction/account_transactions_test.rs +++ b/crates/blockifier/src/transaction/account_transactions_test.rs @@ -174,7 +174,7 @@ fn test_fee_enforcement( ); let account_tx = AccountTransaction::DeployAccount(deploy_account_tx); - let enforce_fee = account_tx.create_tx_info().enforce_fee(); + let enforce_fee = account_tx.create_tx_info().unwrap().enforce_fee(); let result = account_tx.execute(state, &block_context, true, true); assert_eq!(result.is_err(), enforce_fee); } @@ -655,7 +655,7 @@ fn test_fail_declare(block_context: BlockContext, max_fee: Fee) { ); // Fail execution, assert nonce and balance are unchanged. - let tx_info = declare_account_tx.create_tx_info(); + let tx_info = declare_account_tx.create_tx_info().unwrap(); let initial_balance = state .get_fee_token_balance(account_address, chain_info.fee_token_address(&tx_info.fee_type())) .unwrap(); diff --git a/crates/blockifier/src/transaction/errors.rs b/crates/blockifier/src/transaction/errors.rs index cbd9d38085..1a1ab31029 100644 --- a/crates/blockifier/src/transaction/errors.rs +++ b/crates/blockifier/src/transaction/errors.rs @@ -138,3 +138,12 @@ pub enum NumericConversionError { #[error("Conversion of {0} to u128 unsuccessful.")] U128ToUsizeError(u128), } + +// TODO(Nimrod): Delete this error once `create_tx_info` stops returning a `Result`. +#[derive(Debug, Error)] +pub enum TransactionInfoCreationError { + #[error("Invalid ResourceMapping combination was given: {0}")] + InvalidResourceMapping(String), + #[error(transparent)] + StarknetAPIError(#[from] StarknetApiError), +} diff --git a/crates/blockifier/src/transaction/objects.rs b/crates/blockifier/src/transaction/objects.rs index 647a15acfc..0a8a5b4f17 100644 --- a/crates/blockifier/src/transaction/objects.rs +++ b/crates/blockifier/src/transaction/objects.rs @@ -38,6 +38,7 @@ use crate::transaction::constants; use crate::transaction::errors::{ TransactionExecutionError, TransactionFeeError, + TransactionInfoCreationError, TransactionPreValidationError, }; use crate::utils::{u128_from_usize, usize_from_u128}; @@ -565,5 +566,5 @@ pub enum FeeType { } pub trait TransactionInfoCreator { - fn create_tx_info(&self) -> TransactionInfo; + fn create_tx_info(&self) -> Result; } diff --git a/crates/blockifier/src/transaction/post_execution_test.rs b/crates/blockifier/src/transaction/post_execution_test.rs index bb6dd8f43c..271718e7e0 100644 --- a/crates/blockifier/src/transaction/post_execution_test.rs +++ b/crates/blockifier/src/transaction/post_execution_test.rs @@ -117,7 +117,7 @@ fn test_revert_on_overdraft( resource_bounds: max_resource_bounds.clone(), nonce: nonce_manager.next(account_address), }); - let tx_info = approve_tx.create_tx_info(); + let tx_info = approve_tx.create_tx_info().unwrap(); let approval_execution_info = approve_tx.execute(&mut state, &block_context, true, true).unwrap(); assert!(!approval_execution_info.is_reverted()); diff --git a/crates/blockifier/src/transaction/transaction_execution.rs b/crates/blockifier/src/transaction/transaction_execution.rs index 2d4fee1fb2..9ef166b854 100644 --- a/crates/blockifier/src/transaction/transaction_execution.rs +++ b/crates/blockifier/src/transaction/transaction_execution.rs @@ -12,7 +12,7 @@ use crate::fee::actual_cost::TransactionReceipt; use crate::state::cached_state::TransactionalState; use crate::state::state_api::UpdatableState; use crate::transaction::account_transaction::AccountTransaction; -use crate::transaction::errors::TransactionFeeError; +use crate::transaction::errors::{TransactionFeeError, TransactionInfoCreationError}; use crate::transaction::objects::{ TransactionExecutionInfo, TransactionExecutionResult, @@ -100,7 +100,7 @@ impl Transaction { } impl TransactionInfoCreator for Transaction { - fn create_tx_info(&self) -> TransactionInfo { + fn create_tx_info(&self) -> Result { match self { Self::AccountTransaction(account_tx) => account_tx.create_tx_info(), Self::L1HandlerTransaction(l1_handler_tx) => l1_handler_tx.create_tx_info(), diff --git a/crates/blockifier/src/transaction/transactions.rs b/crates/blockifier/src/transaction/transactions.rs index 0d61f2f938..b75965e8e7 100644 --- a/crates/blockifier/src/transaction/transactions.rs +++ b/crates/blockifier/src/transaction/transactions.rs @@ -30,7 +30,7 @@ use crate::state::cached_state::TransactionalState; use crate::state::errors::StateError; use crate::state::state_api::{State, UpdatableState}; use crate::transaction::constants; -use crate::transaction::errors::TransactionExecutionError; +use crate::transaction::errors::{TransactionExecutionError, TransactionInfoCreationError}; use crate::transaction::objects::{ CommonAccountFields, CurrentTransactionInfo, @@ -268,7 +268,7 @@ impl Executable for DeclareTransaction { } impl TransactionInfoCreator for DeclareTransaction { - fn create_tx_info(&self) -> TransactionInfo { + fn create_tx_info(&self) -> Result { // TODO(Nir, 01/11/2023): Consider to move this (from all get_tx_info methods). let common_fields = CommonAccountFields { transaction_hash: self.tx_hash(), @@ -282,27 +282,27 @@ impl TransactionInfoCreator for DeclareTransaction { match &self.tx { starknet_api::transaction::DeclareTransaction::V0(tx) | starknet_api::transaction::DeclareTransaction::V1(tx) => { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields, max_fee: tx.max_fee, - }) + })) } starknet_api::transaction::DeclareTransaction::V2(tx) => { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields, max_fee: tx.max_fee, - }) + })) } starknet_api::transaction::DeclareTransaction::V3(tx) => { - TransactionInfo::Current(CurrentTransactionInfo { + Ok(TransactionInfo::Current(CurrentTransactionInfo { common_fields, - resource_bounds: tx.resource_bounds.clone().try_into().expect("todo"), + resource_bounds: tx.resource_bounds.clone().try_into()?, tip: tx.tip, 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(), - }) + })) } } } @@ -391,7 +391,7 @@ impl Executable for DeployAccountTransaction { } impl TransactionInfoCreator for DeployAccountTransaction { - fn create_tx_info(&self) -> TransactionInfo { + fn create_tx_info(&self) -> Result { let common_fields = CommonAccountFields { transaction_hash: self.tx_hash(), version: self.version(), @@ -403,21 +403,21 @@ impl TransactionInfoCreator for DeployAccountTransaction { match &self.tx() { starknet_api::transaction::DeployAccountTransaction::V1(tx) => { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields, max_fee: tx.max_fee, - }) + })) } starknet_api::transaction::DeployAccountTransaction::V3(tx) => { - TransactionInfo::Current(CurrentTransactionInfo { + Ok(TransactionInfo::Current(CurrentTransactionInfo { common_fields, - resource_bounds: tx.resource_bounds.clone().try_into().expect("todo"), + resource_bounds: tx.resource_bounds.clone().try_into()?, tip: tx.tip, 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: AccountDeploymentData::default(), - }) + })) } } } @@ -509,7 +509,7 @@ impl Executable for InvokeTransaction { } impl TransactionInfoCreator for InvokeTransaction { - fn create_tx_info(&self) -> TransactionInfo { + fn create_tx_info(&self) -> Result { let common_fields = CommonAccountFields { transaction_hash: self.tx_hash(), version: self.version(), @@ -521,27 +521,27 @@ impl TransactionInfoCreator for InvokeTransaction { match &self.tx() { starknet_api::transaction::InvokeTransaction::V0(tx) => { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields, max_fee: tx.max_fee, - }) + })) } starknet_api::transaction::InvokeTransaction::V1(tx) => { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields, max_fee: tx.max_fee, - }) + })) } starknet_api::transaction::InvokeTransaction::V3(tx) => { - TransactionInfo::Current(CurrentTransactionInfo { + Ok(TransactionInfo::Current(CurrentTransactionInfo { common_fields, - resource_bounds: tx.resource_bounds.clone().try_into().expect("todo"), + resource_bounds: tx.resource_bounds.clone().try_into()?, tip: tx.tip, 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(), - }) + })) } } } @@ -607,8 +607,8 @@ impl Executable for L1HandlerTransaction { } impl TransactionInfoCreator for L1HandlerTransaction { - fn create_tx_info(&self) -> TransactionInfo { - TransactionInfo::Deprecated(DeprecatedTransactionInfo { + fn create_tx_info(&self) -> Result { + Ok(TransactionInfo::Deprecated(DeprecatedTransactionInfo { common_fields: CommonAccountFields { transaction_hash: self.tx_hash, version: self.tx.version, @@ -618,6 +618,6 @@ impl TransactionInfoCreator for L1HandlerTransaction { only_query: false, }, max_fee: Fee::default(), - }) + })) } } diff --git a/crates/native_blockifier/src/py_validator.rs b/crates/native_blockifier/src/py_validator.rs index e035f0c4ef..4b6f1e1896 100644 --- a/crates/native_blockifier/src/py_validator.rs +++ b/crates/native_blockifier/src/py_validator.rs @@ -91,7 +91,7 @@ impl PyValidator { if account_tx.tx_type() != TransactionType::InvokeFunction { return Ok(false); } - let tx_info = account_tx.create_tx_info(); + let tx_info = account_tx.create_tx_info()?; let nonce = self.stateful_validator.get_nonce(tx_info.sender_address())?; let deploy_account_not_processed =