starknet_os: allocate and replace aliases test

[nimrod-starkware]

No description provided.

[nimrod-starkware]

• starknet_os: allocate and replace aliases test #7745 : 2 dependent PRs (#7765 , #7792 ) 👈 (View in Graphite)
• starknet_os: refactor cairo runner utils #7744
• starknet_os: pointer args wrap Vec<MaybeRelocatable> #7743
• main-v0.14.0

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

[reviewable-StarkWare]

This change is 

[dorimedini-starkware]

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

---

crates/starknet_os/src/test_utils/utils.rs line 61 at r1 (raw file):

    let dict_segment_end = vm.load_data(dict_segment_start, &squashed_dict).unwrap();
    (dict_segment_start, dict_segment_end)
}

this function now actually allocates a segment and loads data into it - you should probably rename it + doc it

Suggestion:

/// Creates a squashed dict from previous and new values, and stores it in a new memory segment.
pub fn allocate_squashed_cairo_dict(
    prev_values: &HashMap<Felt, MaybeRelocatable>,
    new_values: &HashMap<Felt, MaybeRelocatable>,
    vm: &mut VirtualMachine,
) -> (Relocatable, Relocatable) {
    let squashed_dict = flatten_cairo_dict(prev_values, new_values);
    let dict_segment_start = vm.add_memory_segment();
    let dict_segment_end = vm.load_data(dict_segment_start, &squashed_dict).unwrap();
    (dict_segment_start, dict_segment_end)
}

---

crates/starknet_os/src/tests/aliases.rs line 439 at r1 (raw file):

            10000,
            HashMap::from([(34567, 0), (435, 0)])
        )

Suggestion:

        (
            1,
            HashMap::from([(777, 1), (8888, 1), (9999, 1)]) // No aliases.
        ),
        (
            100,
            HashMap::from([(200, 1), (777, 1), (888, 0)]) // Aliases for non-trivial diffs.
        ),
        (
            600,
            HashMap::from([(2000, 1), (3000, 1)])
        ),
        (
            800,
            HashMap::from([(700, 1), (701, 1)])
        ),
        (
            3000,
            HashMap::from([(600, 1), (2000, 1)])
        ),
        (
            10000,
            HashMap::from([(34567, 0), (435, 0)]) // No aliases (all diffs trivial).
        )

---

crates/starknet_os/src/tests/aliases.rs line 536 at r1 (raw file):

    .unwrap();

    // TODO(Nimrod): Complete this test to also compare the other return values.

may be hard to remember by the time you do it.
if it's already a WIP - ignore this :)

Suggestion:

// TODO(Nimrod): Complete this test to also compare the other return values (state diff and
//   state diff with aliases).

---

crates/starknet_os/src/tests/aliases.rs line 555 at r1 (raw file):

            "The return value doesn't match the given format.\n Got: {explicit_return_values:?}"
        );
    }

I think let..else pattern is better here, non-blocking

Suggestion:

    let [
        EndpointArg::Pointer(PointerArg::Array(aliases_storage_updates)),
        EndpointArg::Pointer(PointerArg::Array(_)),
        EndpointArg::Pointer(PointerArg::Array(_)),
    ] = explicit_return_values.as_slice() else {
        panic!(
            "The return value doesn't match the given format.\n Got: {explicit_return_values:?}"
        );
    };
    let aliases_storage_updates_as_felts: Vec<Felt> =
        aliases_storage_updates.iter().map(|f| f.get_int().unwrap()).collect();
    let actual_alias_storage = parse_squashed_cairo_dict(&aliases_storage_updates_as_felts);
    let expected_alias_storage: HashMap<Felt, Felt> = expected_alias_storage
        .into_iter()
        .map(|(key, value)| (key.into(), value.into()))
        .collect();
    assert_eq!(actual_alias_storage, expected_alias_storage);

[nimrod-starkware]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @amosStarkware and @dorimedini-starkware)

---

crates/starknet_os/src/test_utils/utils.rs line 61 at r1 (raw file):

Previously, dorimedini-starkware wrote…

this function now actually allocates a segment and loads data into it - you should probably rename it + doc it

Done.

---

crates/starknet_os/src/tests/aliases.rs line 555 at r1 (raw file):

Previously, dorimedini-starkware wrote…

I think let..else pattern is better here, non-blocking

Done.

---

crates/starknet_os/src/tests/aliases.rs line 439 at r1 (raw file):

            10000,
            HashMap::from([(34567, 0), (435, 0)])
        )

Done.

[dorimedini-starkware]

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @amosStarkware)

[dorimedini-starkware]

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