test(concurrency): add utilities for commit tx test

[meship-starkware]

This change is 

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.32%. Comparing base (952373d) to head (192388a). Report is 9 commits behind head on main.

Files	Patch %	Lines
crates/blockifier/src/fee/fee_utils.rs	75.00%	1 Missing and 1 partial :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #1922 +/- ## ========================================== + Coverage 78.17% 78.32% +0.15% ========================================== Files 62 62 Lines 8842 9279 +437 Branches 8842 9279 +437 ========================================== + Hits 6912 7268 +356 - Misses 1492 1564 +72 - Partials 438 447 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[avi-starkware]

crates/blockifier/src/concurrency/fee_utils.rs line 11 at r1 (raw file):

// `Uint256`, consist of two felts (lsb, msb). Hence, storage read values =
// [account_balance, 0, sequencer_balance, 0]
pub const STORAGE_READ_SEQUENCER_BALANCE_INDICES: (usize, usize) = (2, 3);

please make this change in #1923, where the constant is used.

Code quote:

pub const STORAGE_READ_SEQUENCER_BALANCE_INDICES: (usize, usize) = (2, 3);

[avi-starkware]

crates/blockifier/src/concurrency/test_utils.rs line 71 at r1 (raw file):

) -> CallInfo {
    let block_context = BlockContext::create_for_testing_with_concurrency_mode(concurrency_mode);
    let mut transactional_state = CachedState::create_transactional(state);

Why change it? Isn't it the same method?

Code quote:

    let mut transactional_state = CachedState::create_transactional(state);

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic.rs line 31 at r1 (raw file):

const EXECUTION_OUTPUTS_UNWRAP_ERROR: &str = "Execution task outputs should not be None.";
#[derive(Debug)]

Restore the empty line.

Suggestion:

const EXECUTION_OUTPUTS_UNWRAP_ERROR: &str = "Execution task outputs should not be None.";

#[derive(Debug)]

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 47 at r1 (raw file):

}

fn _sequencer_transfer_invoke_tx(

Transfer is always invoke

Suggestion:

fn _transfer_to_sequencer_tx(

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 57 at r1 (raw file):

        block_context.chain_info().fee_token_address(&FeeType::Strk),
        TRANSFER_ENTRY_POINT_NAME,
        &[*sequencer_address.0.key(), stark_felt!(transfer_amount), stark_felt!(0_u8)],

Suggestion:

        &[
            *sequencer_address.0.key(),     // Recipient address.
            stark_felt!(transfer_amount),   // Amount low.
            stark_felt!(0_u8)               // Amount high.
        ],

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 88 at r1 (raw file):

    account_address: ContractAddress,
    tx_index: usize,
    expected_storage_values: (StarkFelt, StarkFelt, StarkFelt, StarkFelt),

You are allowed 7 arguments. Let's make it easier to understand what the input of this function should be.

• Alternatively, you can add a comment explaining what each felt represents.

Suggestion:

    account_balance_low: StarkFelt,
    account_balance_high: StarkFelt,
    sequencer_balance_low: StarkFelt,
    sequencer_balance_high: StarkFelt,

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 88 at r1 (raw file):

Previously, avi-starkware wrote…

You are allowed 7 arguments. Let's make it easier to understand what the input of this function should be. * Alternatively, you can add a comment explaining what each felt represents.

Sorry, please add the prefix expected_ to each of the argument names I proposed.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 70 at r1 (raw file):

}

fn _invoke_tx_without_calldata(

I liked the name invalid transaction. It makes it easy to understand what it is going to be used for. I think it's better to explain why it is invalid in a comment to make it easy to understand for someone who is reading the function for the first time.

Suggestion:

/// Creates an invalid transaction. The transaction has no calldata, making it fail validation<is that where it fails?>.
fn _invalid_tx(

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 112 at r1 (raw file):

        );
    }
}

Suggestion:

    let (sequencer_balance_key_low, sequencer_balance_key_high) =
        get_sequencer_balance_keys(&executor.block_context);
    for (expected_balance, storage_key) in [
        (expected_storage_values.0, account_balance_key_low),
        (expected_storage_values.1, account_balance_key_high),
        (expected_storage_values.2, sequencer_balance_key_low),
        (expected_storage_values.3, sequencer_balance_key_high),
    ] {
        let actual_balance = tx_version_state
            .get_storage_at(
                executor.block_context.chain_info.fee_token_address(&FeeType::Strk),
                storage_key
            )
            .unwrap();
        assert_eq!(actual_balance, expected_balance);
    }
}

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 114 at r2 (raw file):

}

fn _write_account_balance_at_versioned_state(

Sorry, I know I proposed the name, but I think it is confusing.

Suggestion:

fn _change_account_balance_at_version(

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 114 at r2 (raw file):

Previously, avi-starkware wrote…

Sorry, I know I proposed the name, but I think it is confusing.

Maybe add a dock string to make this function clearer:

Code snippet:

/// Changes an account's balance observed by transaction `tx_index`. Used to cause a transaction to 
/// either pass or fail validation in tests.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 61 at r4 (raw file):

            stark_felt!(transfer_amount), // Amount low.
            stark_felt!(0_u8),
        ], // Amount high.

Suggestion:

            stark_felt!(0_u8),      // Amount high.
        ],

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 105 at r4 (raw file):

    for (expected_balance, storage_key) in [
        (expected_account_balance_low, account_balance_key_low),
        // In tests we assume accout balane can be represented as u128.

Suggestion:

 // In tests, we assume that the account balance can be represented as u128.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 114 at r2 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…

Done.

What do you mean , or filling the account balance high.? I looked at all the use cases in #1893, and all I saw was either making a transaction fail validation or pass validation.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 45 at r10 (raw file):

    nonce: Nonce,
) -> AccountTransaction {
    // let some_other_account_address = account_contract.get_instance_address(17);

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 40 at r10 (raw file):

use crate::{declare_tx_args, invoke_tx_args, nonce, storage_key};

fn _trivial_transfer_tx(

Suggestion:

// Creates a tx that transfers 0 to self.
fn _trivial_transfer_tx(

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 69 at r10 (raw file):

    executor: &WorkerExecutor<'_, S>,
    tx_index: usize,
    // We assume balance < 2^128.

Suggestion:

    // We assume the balance is at most 2^128, so the "low" value is sufficient.

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 66 at r10 (raw file):

/// Checks that the storage values of the account and sequencer balances in the
/// versioned state of tx_index equals the expected values.
fn _verify_sequencer_balance_update<S: StateReader>(

Retracted. The old name is a better fit for the changes you make to this function in the next PR.