starkware-libs / blockifier

Blockifier is a Rust implementation for the transaction-executing component in the StarkNet sequencer, in charge of creating state diffs and blocks.
Apache License 2.0
170 stars 107 forks source link

test(concurrency): test concurrent fee transfer when the sequencer is… #1963

Closed meship-starkware closed 2 months ago

meship-starkware commented 3 months ago

… the sender


This change is Reviewable

codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 78.68%. Comparing base (6e06e79) to head (8995b60). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1963 +/- ## ========================================== + Coverage 78.42% 78.68% +0.25% ========================================== Files 62 62 Lines 8913 8988 +75 Branches 8913 8988 +75 ========================================== + Hits 6990 7072 +82 + Misses 1476 1464 -12 - Partials 447 452 +5 ```

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

avi-starkware commented 3 months ago

crates/blockifier/src/transaction/account_transactions_test.rs line 1269 at r1 (raw file):

    block_context.block_info = block_info;

    // Create the account transaction accurding to the fee type.

Suggestion:

    // Create the account transaction according to the fee type.
avi-starkware commented 3 months ago

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…
I think the first assertion is enough because it indicates that we run the sequential fee transfer, and a test already checks it. WDYT?

You want to remove the second assertion?

Code snippet:

            assert_eq!(
                *storage.get(&(fee_token_address, seq_key)).unwrap(),
                stark_felt!(seq_value)
            );
avi-starkware commented 3 months ago

crates/blockifier/src/transaction/account_transactions_test.rs line 1302 at r1 (raw file):

Previously, meship-starkware (Meshi Peled) wrote…
Yes.

Actually, I think we should remove the first assertion. If we are able to compare the value at the key, then that key obviously exists.

avi-starkware commented 2 months ago

crates/blockifier/src/transaction/account_transactions_test.rs line 1247 at r7 (raw file):

    assert!(!result.is_reverted());

    // Check that the sequencer balance was updared (in this case, was not changed).

Suggestion:

    // Check that the sequencer balance was updated (in this case, was not changed).