test(starknet_state_sync): add unit tests for state sync read methods

[noamsp-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[github-actions]

Artifacts upload workflows:

• Started at 2024-12-22T18:46:09Z
• Started at 2024-12-22T18:50:32Z
• Started at 2024-12-23T18:14:31Z
• Started at 2024-12-24T11:27:24Z
• Started at 2024-12-25T14:52:04Z
• Started at 2024-12-26T10:15:17Z
• Started at 2024-12-26T11:52:46Z
• Started at 2024-12-26T13:14:08Z
• Started at 2024-12-26T13:22:33Z

[noamsp-starkware]

• test(starknet_state_sync): add unit tests for state sync read methods #2886 👈 (View in Graphite)
• fix(starknet_state_sync): verify contract deployed at get storage/nonce #2963 : 1 other dependent PR (#2942 )
• fix(starknet_state_sync): change marker verify from casm to state #2979
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[ShahakShama]

Reviewed 4 of 5 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @noamsp-starkware)

---

crates/starknet_state_sync/src/test.rs line 23 at r2 (raw file):

use crate::StateSync;

fn get_mock_state_sync_and_storage_writer() -> (StateSync, StorageWriter) {

Why is this mock? It's the real thing just without the new_block_sender
I'd rename this function to setup

---

crates/starknet_state_sync/src/test.rs line 40 at r2 (raw file):

        .append_header(header.block_header_without_hash.block_number, &header)
        .unwrap()
        .append_state_diff(header.block_header_without_hash.block_number, ThinStateDiff::default())

Put a non default ThinStateDiff. You can use get_test_instance. It makes for a more interesting test

---

crates/starknet_state_sync/src/test.rs line 49 at r2 (raw file):

    // Verify that the block that was written is returned correctly.
    let value =
        state_sync.get_block(header.block_header_without_hash.block_number).unwrap().unwrap();

I prefer using the external API: state_sync.handle_request(StateSyncRequest::GetBlock(...))
Same across the entire PR

---

crates/starknet_state_sync/src/test.rs line 56 at r2 (raw file):

    // Verify we get Ok(None) when trying to call get_block with an unknown block number.
    let result = state_sync

I usually prefer doing separate test cases in separate tests. This way you put these comments in the test title
In most cases here you don't need any setup for the special case. You can just use an empty storage with any block number to test this case

Same across this PR

---

crates/starknet_state_sync/src/test.rs line 68 at r2 (raw file):

    let mut rng = get_rng();
    let expected_address_u64 = rng.next_u64();
    let expected_address = ContractAddress::from(expected_address_u64);

expected_* is usually used for the expected return value. address is an argument and not a return value. You can just call it address

Same across this PR

---

crates/starknet_state_sync/src/test.rs line 94 at r2 (raw file):

    // number.
    let result = state_sync.get_storage_at(
        header.block_header_without_hash.block_number.unchecked_next(),

Consider keeping this in a var called wrong_block_number or non_existing_block_number or other_block_number and use it here and in the assert

---

crates/starknet_state_sync/src/test.rs line 123 at r2 (raw file):

    let expected_address_u64 = rng.next_u64();
    let expected_address = ContractAddress::from(expected_address_u64);
    let expected_value = Nonce::get_test_instance(&mut rng);

value -> nonce

---

crates/starknet_state_sync/src/test.rs line 172 at r2 (raw file):

    let expected_address_u64 = rng.next_u64();
    let expected_address = ContractAddress::from(expected_address_u64);
    let expected_value = ClassHash::get_test_instance(&mut rng);

value -> class_hash

---

crates/starknet_state_sync/src/test.rs line 215 at r2 (raw file):

#[tokio::test]
async fn test_get_compiled_class_deprecated() {

You can avoid updating this test according to my comments since it will be erased soon

[noamsp-starkware]

Reviewable status: 4 of 5 files reviewed, 7 unresolved discussions (waiting on @ShahakShama)

---

crates/starknet_state_sync/src/test.rs line 23 at r2 (raw file):

Previously, ShahakShama wrote…

Why is this mock? It's the real thing just without the new_block_sender
I'd rename this function to setup

Missed this, its from previous trials when writing the file. fixing

---

crates/starknet_state_sync/src/test.rs line 40 at r2 (raw file):

Previously, ShahakShama wrote…

Put a non default ThinStateDiff. You can use get_test_instance. It makes for a more interesting test

Done.

---

crates/starknet_state_sync/src/test.rs line 49 at r2 (raw file):

Previously, ShahakShama wrote…

I prefer using the external API: state_sync.handle_request(StateSyncRequest::GetBlock(...))
Same across the entire PR

Done.

---

crates/starknet_state_sync/src/test.rs line 56 at r2 (raw file):

Previously, ShahakShama wrote…

I usually prefer doing separate test cases in separate tests. This way you put these comments in the test title
In most cases here you don't need any setup for the special case. You can just use an empty storage with any block number to test this case

Same across this PR

Done.

---

crates/starknet_state_sync/src/test.rs line 68 at r2 (raw file):

Previously, ShahakShama wrote…

expected_* is usually used for the expected return value. address is an argument and not a return value. You can just call it address

Same across this PR

Done.

---

crates/starknet_state_sync/src/test.rs line 123 at r2 (raw file):

Previously, ShahakShama wrote…

value -> nonce

Done.

---

crates/starknet_state_sync/src/test.rs line 172 at r2 (raw file):

Previously, ShahakShama wrote…

value -> class_hash

Done.

[ShahakShama]

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 7 unresolved discussions (waiting on @noamsp-starkware)

---

crates/starknet_state_sync/src/test.rs line 65 at r3 (raw file):

        }
        StateSyncResponse::GetBlock(Ok(None)) => {
            panic!("No block returned");

No need to differentiate this case if you accept my suggestions below.
Same across the entire PR

---

crates/starknet_state_sync/src/test.rs line 68 at r3 (raw file):

        }
        _ => {
            panic!("Expected StateSyncResponse::GetBlock::Ok");

display the response you actually got inside the panic message.
Same across the entire PR

---

crates/starknet_state_sync/src/test.rs line 68 at r3 (raw file):

        }
        _ => {
            panic!("Expected StateSyncResponse::GetBlock::Ok");

If you apply the 2 suggestions above, change StateSyncResponse::GetBlock::Ok to StateSyncResponse::GetBlock::Ok(Some(_))
Same across the entire PR

---

crates/starknet_state_sync/src/test.rs line 252 at r3 (raw file):

async fn test_block_not_found() {
    let (mut state_sync, _) = setup();
    let wrong_block_number = BlockNumber(0);

consider non_existing_block_number

---

crates/starknet_state_sync/src/test.rs line 325 at r3 (raw file):

    let address = ContractAddress::from(1u64);
    let mut diff = ThinStateDiff::default();
    diff.storage_diffs.insert(address, IndexMap::from([(Default::default(), Default::default())]));

Shouldn't you do the same for nonces?

---

crates/starknet_state_sync/src/test.rs line 325 at r3 (raw file):

    let address = ContractAddress::from(1u64);
    let mut diff = ThinStateDiff::default();
    diff.storage_diffs.insert(address, IndexMap::from([(Default::default(), Default::default())]));

Add a comment that you're creating a corrupted state diff (storage diffs for a contract that wasn't deployed) and you're checking that even if the state diff is corrupted the function will work as expected

---

crates/starknet_state_sync/src/test.rs line 48 at r2 (raw file):

    // Verify that the block that was written is returned correctly.
    let value =

use let else instead of match for a more compact syntax
Same across the entire PR

[noamsp-starkware]

Reviewable status: 4 of 5 files reviewed, 6 unresolved discussions (waiting on @ShahakShama)

---

crates/starknet_state_sync/src/test.rs line 48 at r2 (raw file):

Previously, ShahakShama wrote…

use let else instead of match for a more compact syntax
Same across the entire PR

Done.

---

crates/starknet_state_sync/src/test.rs line 65 at r3 (raw file):

Previously, ShahakShama wrote…

No need to differentiate this case if you accept my suggestions below.
Same across the entire PR

Done.

---

crates/starknet_state_sync/src/test.rs line 68 at r3 (raw file):

Previously, ShahakShama wrote…

display the response you actually got inside the panic message.
Same across the entire PR

Done.

---

crates/starknet_state_sync/src/test.rs line 68 at r3 (raw file):

Previously, ShahakShama wrote…

If you apply the 2 suggestions above, change StateSyncResponse::GetBlock::Ok to StateSyncResponse::GetBlock::Ok(Some(_))
Same across the entire PR

Done.

---

crates/starknet_state_sync/src/test.rs line 325 at r3 (raw file):

Previously, ShahakShama wrote…

Shouldn't you do the same for nonces?

Ill add it just for consistency, but to be honest, we don't really need this since we are just looking for an address that doesn't exist (therefore empty state_diff also does the job)

---

crates/starknet_state_sync/src/test.rs line 325 at r3 (raw file):

Previously, ShahakShama wrote…

Add a comment that you're creating a corrupted state diff (storage diffs for a contract that wasn't deployed) and you're checking that even if the state diff is corrupted the function will work as expected

Done.

[ShahakShama]

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

---

crates/starknet_state_sync/src/test.rs line 327 at r4 (raw file):

        .unwrap();

    // Check that get_storage_at verifies the contract was not deployed and therefore returns

Add to the end of the sentence ", even though the state diff is corrupt and contains this storage"

---

crates/starknet_state_sync/src/test.rs line 342 at r4 (raw file):

    assert_eq!(get_storage_at_result, Err(StateSyncError::ContractNotFound(address)));

    // Check that calling the functions below with a non existing adress returns contract not found

Put the same comment you did in get_storage_at plus my suggestion

[noamsp-starkware]

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @ShahakShama)

---

crates/starknet_state_sync/src/test.rs line 327 at r4 (raw file):

Previously, ShahakShama wrote…

Add to the end of the sentence ", even though the state diff is corrupt and contains this storage"

Done.

---

crates/starknet_state_sync/src/test.rs line 342 at r4 (raw file):

Previously, ShahakShama wrote…

Put the same comment you did in get_storage_at plus my suggestion

Added this to the comment above, thought it looked better

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.958 ms 34.007 ms 34.062 ms]
change: [-4.5849% -3.0229% -1.6483%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
8 (8.00%) high mild
1 (1.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [33.900 ms 33.931 ms 33.969 ms]
change: [-3.9186% -2.3701% -1.0434%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe

[eitanm-starkware]

Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama)

[noamsp-starkware]

Dismissed @ShahakShama from 2 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware)

[ShahakShama]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware)