test(concurrency): test commit tx where the sequencer address is als…

[meship-starkware]

…o the sender address

---

This change is 

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.35%. Comparing base (cefb2a2) to head (cff9e85).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1935   +/-   ##
=======================================
  Coverage   78.35%   78.35%           
=======================================
  Files          62       62           
  Lines        8861     8861           
  Branches     8861     8861           
=======================================
  Hits         6943     6943           
  Misses       1479     1479           
  Partials      439      439           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[avi-starkware]

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

}

fn check_sequencer_sender_fee_transfer<S: StateReader>(

Make the name more similar to validate_fee_transfer to make it clear that it does the same thing only that the sender is the sequencer.

Suggestion:

fn validate_fee_transfer_when_sender_is_sequencer<S: StateReader>(

[avi-starkware]

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

#[test]
fn test_commit_tx_when_the_sqeuncer_is_the_sender() {

Add a docstring explaining why we need to check this case specifically. (The thing with skipping the last part of the commit in this case.)

Suggestion:

fn test_commit_tx_when_sender_is_sequencer() {

[avi-starkware]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

[avi-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

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

Previously, avi-starkware wrote…

Make the name more similar to validate_fee_transfer to make it clear that it does the same thing only that the sender is the sequencer.

Done.

---

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

Previously, avi-starkware wrote…

Add a docstring explaining why we need to check this case specifically. (The thing with skipping the last part of the commit in this case.)

Done.

[avi-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 130 at r6 (raw file):

        .unwrap();

    // Commit tx and check that the commit did not affected the result and state.

Suggestion:

    // Commit tx and check that the commit made no changes in the execution result or the state.

[avi-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 100 at r6 (raw file):

        account_address,
        nonce_manager.next(account_address),
    ))];

If there is only one transaction, there is no need to put it in an array

Code quote:

    let txs = [Transaction::AccountTransaction(trivial_calldata_invoke_tx(
        account_address,
        nonce_manager.next(account_address),
    ))];

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 103 at r6 (raw file):

    let cached_state =
        test_state(&block_context.chain_info, BALANCE, &[(account, txs.len().try_into().unwrap())]);

Suggestion:

    let cached_state =
        test_state(&block_context.chain_info, BALANCE, &[(account, 1)]);

[meship-starkware]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @avi-starkware and @noaov1)

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 100 at r6 (raw file):

Previously, avi-starkware wrote…

If there is only one transaction, there is no need to put it in an array

The worker executer expects a transaction array

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 103 at r6 (raw file):

    let cached_state =
        test_state(&block_context.chain_info, BALANCE, &[(account, txs.len().try_into().unwrap())]);

Done.

---

crates/blockifier/src/concurrency/worker_logic_test.rs line 130 at r6 (raw file):

        .unwrap();

    // Commit tx and check that the commit did not affected the result and state.

Done.

[avi-starkware]

Reviewed 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

crates/blockifier/src/concurrency/worker_logic_test.rs line 136 at r7 (raw file):

        commit_result.as_ref().unwrap().fee_transfer_call_info.as_ref().unwrap();
    assert_eq!(read_values_before_commit, fee_transfer_call_info.storage_read_values);
    drop(execution_task_outputs);

No need to drop it right?

Code quote:

    drop(execution_task_outputs);

[avi-starkware]

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[avi-starkware]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)

[noaov1]

Reviewed 1 of 2 files at r1, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @meship-starkware)

---

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

    let mut block_info = block_context.block_info.clone();
    block_info.sequencer_address = account_address;
    block_context.block_info = block_info;

Suggestion:

    block_context.block_info.sequencer_address = account_address;

---

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

        test_contract_address,
        nonce_manager.next(account_address),
    ))];

Is the nonce manager needed?

Suggestion:

    let tx = Transaction::AccountTransaction(trivial_calldata_invoke_tx(
        account_address,
        test_contract_address,
        nonce_manager.next(account_address),
    ));

---

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

    let cached_state =
        test_state(&block_context.chain_info, BALANCE, &[(account, 1), (test_contract, 1)]);

Suggestion:

    let state =
        test_state(&block_context.chain_info, BALANCE, &[(account, 1), (test_contract, 1)]);

---

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

    let sequencer_balance_high_before = tx_versioned_state
        .get_storage_at(
            executor.block_context.chain_info.fee_token_address(&FeeType::Strk),

Please define a variable and extract the fee type from the transaction context

Code quote:

 executor.block_context.chain_info.fee_token_address(&FeeType::Strk)

The base branch was changed.

[meship-starkware]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @noaov1)

---

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

Previously, noaov1 (Noa Oved) wrote…

Is the nonce manager needed?

The executor expects a list of txes, so if I give it only one tx, it won't work.
The nonce manager is not needed.

---

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

    let cached_state =
        test_state(&block_context.chain_info, BALANCE, &[(account, 1), (test_contract, 1)]);

Done.

---

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

Previously, noaov1 (Noa Oved) wrote…

Please define a variable and extract the fee type from the transaction context

Done.

[noaov1]

Reviewed 1 of 2 files at r6, 1 of 1 files at r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @meship-starkware)