SimulateTransactions integration tests #628
Conversation
| @@ -1210,6 +1153,7 @@ impl Starknet { | |||
| transactions, | |||
| Some(!skip_fee_charge), | |||
| Some(!skip_validate), | |||
| false, | |||
There was a problem hiding this comment.
Why should return_error_on_reverted_execution be false here? Won't the error have already occurred on blockifier_transaction.execute in L1126?
|
|
||
| match estimate_fee_result { | ||
| Ok(estimated_fee) => Ok(estimated_fee), | ||
| Err(Error::UnexpectedInternalError { msg }) => Err(Error::ExecutionError { execution_error: msg, index: idx }), |
There was a problem hiding this comment.
Should ExecutionError (used by RPC) be mapped into UnexpectedInternalError? When do we even get UnexpectedInternalError? Isn't it a way to catch those unfortunate conversions that never fail?
| impl From<TransactionExecutionError> for Error { | ||
| fn from(value: TransactionExecutionError) -> Self { | ||
| match value { | ||
| blockifier::transaction::errors::TransactionExecutionError::TransactionPreValidationError( |
There was a problem hiding this comment.
Why fully qualify the error if it's already imported at the top of the file? Applicable to other new impl From sections in this file.
| @@ -178,7 +182,7 @@ mod old_state { | |||
| .await | |||
There was a problem hiding this comment.
Could the block_id defined in L165 be used in L180?
| async fn estimate_fee_and_simulate_transaction_for_contract_deployment_in_an_old_block_should_produce_the_same_error() | ||
| async fn estimate_fee_and_simulate_transaction_for_contract_deployment_in_an_old_block_should_not_produce_the_same_error() |
There was a problem hiding this comment.
Should it or should it not? starknet_simulateTransactions can return TRANSACTION_EXECUTION_ERROR according to the spec, but in what case if not in this? I think this is a difference between your PR and mine (#627).
| @@ -578,4 +583,201 @@ mod estimate_fee_tests { | |||
| .iter() | |||
| .for_each(assert_fee_estimation); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn estimate_fee_of_multiple_txs_should_return_correct_index_if_one_of_them_fails() { | |||
There was a problem hiding this comment.
This test name is not discriminative enough, compared with the other test you added in this file, since it could also be applied there.
| if skip_fee_charge { | ||
| simulation_result.unwrap(); | ||
| } else { | ||
| match simulation_result.unwrap_err() { | ||
| AccountError::Provider(ProviderError::StarknetError( | ||
| StarknetError::TransactionExecutionError(TransactionExecutionErrorData { | ||
| execution_error, | ||
| .. | ||
| }), | ||
| )) => { | ||
| assert_contains(&execution_error, "max fee is not enough"); | ||
| } | ||
| other_error => panic!("Unexpected error {other_error:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
This suggestion uses less lines and avoids matching inside an if-else construct.
| if skip_fee_charge { | |
| simulation_result.unwrap(); | |
| } else { | |
| match simulation_result.unwrap_err() { | |
| AccountError::Provider(ProviderError::StarknetError( | |
| StarknetError::TransactionExecutionError(TransactionExecutionErrorData { | |
| execution_error, | |
| .. | |
| }), | |
| )) => { | |
| assert_contains(&execution_error, "max fee is not enough"); | |
| } | |
| other_error => panic!("Unexpected error {other_error:?}"), | |
| } | |
| } | |
| match (simulation_result, skip_fee_charge) { | |
| (Ok(_), true) => (), | |
| ( | |
| Err(AccountError::Provider(ProviderError::StarknetError( | |
| StarknetError::TransactionExecutionError(TransactionExecutionErrorData { | |
| execution_error, | |
| .. | |
| }), | |
| ))), | |
| false, | |
| ) => assert_contains(&execution_error, "max fee is not enough"), | |
| invalid_combination => panic!("Invalid combination: {invalid_combination:?}"), | |
| } |
| let chain_id = devnet.json_rpc_client.chain_id().await.unwrap(); | ||
| let paymaster_data = vec![]; | ||
| let tip = 0; | ||
| let gas = 0; |
There was a problem hiding this comment.
Why does it work with gas set to 0 and validation not skipped?
|
|
||
| let salt = Felt::from_hex_unchecked("0x123"); | ||
|
|
||
| account |
There was a problem hiding this comment.
There is a function in commons/utils.rs that abstracts declaring and deploying. Currently it does so relying on v1, but that shouldn't matter in this and similar cases in this file, because the declaring and deploying don't need to be v3 if we're later just testing a panic behavior in an invoke v3.
Development related changes
Checklist:
./scripts/format.sh./scripts/clippy_check.sh./scripts/check_unused_deps.sh./scripts/check_spelling.sh./website/README.md