Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator Author

dorimedini-starkware commented Jun 5, 2025

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

Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/starknet_os/src/hints/enum_definition_test.rs line 140 at r1 (raw file):

                !matches!(
                    selector,
                    // As the new and deprecated syscall selector enums are the same enum,

whyyy

Code quote:

// As the new and deprecated syscall selector enums are the same enum,

crates/starknet_os/src/hints/enum_definition_test.rs line 181 at r1 (raw file):

         syscall hints. Converted selectors: {converted_selectors:#?}, hints: \
         {deprecated_syscall_hints:#?}"
    );

you're verifying that every deprecated sycall has exactly one hint - I was thinking of verifying that DeprecatedSyscallHint contains exactly the syscall hints used by the OS.
If we know every syscall will have exactly one hint, and the hint string will not change, I guess that's ok. is this the case?
if yes - consider explaining this in a comment

Code quote:

    assert_eq!(
        deprecated_syscall_selectors.len(),
        deprecated_syscall_hints.len(),
        "The number of deprecated syscall selectors does not match the number of deprecated \
         syscall hints. Selectors: {deprecated_syscall_selectors:#?}, hints: \
         {deprecated_syscall_hints:#?}",
    );

    let converted_selectors: HashSet<DeprecatedSyscallHint> = deprecated_syscall_selectors
        .iter()
        .map(|selector| DeprecatedSyscallHint::try_from(*selector).unwrap())
        .collect();
    assert_eq!(
        converted_selectors,
        deprecated_syscall_hints.iter().cloned().collect(),
        "The deprecated syscall selectors, converted to hints, do not match the deprecated \
         syscall hints. Converted selectors: {converted_selectors:#?}, hints: \
         {deprecated_syscall_hints:#?}"
    );

Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware)


crates/starknet_os/src/hints/enum_definition_test.rs line 140 at r1 (raw file):

Previously, amosStarkware wrote…

whyyy

lol yes... bummer


crates/starknet_os/src/hints/enum_definition_test.rs line 181 at r1 (raw file):

Previously, amosStarkware wrote…

you're verifying that every deprecated sycall has exactly one hint - I was thinking of verifying that DeprecatedSyscallHint contains exactly the syscall hints used by the OS.
If we know every syscall will have exactly one hint, and the hint string will not change, I guess that's ok. is this the case?
if yes - consider explaining this in a comment

hint strings are tested in the next PR, is that what you meant?

Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)


crates/starknet_os/src/hints/enum_definition_test.rs line 181 at r1 (raw file):

Previously, dorimedini-starkware wrote…

hint strings are tested in the next PR, is that what you meant?

yes

@dorimedini-starkware dorimedini-starkware force-pushed the 06-03-test_starknet_os_hint_consistency_test_with_local_program_objects branch from 63043ed to 4cd582d Compare June 5, 2025 13:02
@dorimedini-starkware dorimedini-starkware force-pushed the 06-05-test_starknet_os_deprecated_syscall_hint_consistency_test branch from fc9c69e to a9db393 Compare June 5, 2025 13:03
@dorimedini-starkware dorimedini-starkware force-pushed the 06-03-test_starknet_os_hint_consistency_test_with_local_program_objects branch from 4cd582d to b5cbf8d Compare June 5, 2025 13:52
@dorimedini-starkware dorimedini-starkware force-pushed the 06-05-test_starknet_os_deprecated_syscall_hint_consistency_test branch from a9db393 to 4ed5535 Compare June 5, 2025 13:52
@dorimedini-starkware dorimedini-starkware force-pushed the 06-03-test_starknet_os_hint_consistency_test_with_local_program_objects branch from b5cbf8d to ef0e32d Compare June 5, 2025 17:08
@dorimedini-starkware dorimedini-starkware force-pushed the 06-05-test_starknet_os_deprecated_syscall_hint_consistency_test branch from 4ed5535 to eddb322 Compare June 5, 2025 17:08
@dorimedini-starkware dorimedini-starkware force-pushed the 06-03-test_starknet_os_hint_consistency_test_with_local_program_objects branch from ef0e32d to 168e351 Compare June 8, 2025 08:57
@dorimedini-starkware dorimedini-starkware force-pushed the 06-05-test_starknet_os_deprecated_syscall_hint_consistency_test branch from eddb322 to e534e35 Compare June 8, 2025 08:57
@dorimedini-starkware dorimedini-starkware changed the base branch from 06-03-test_starknet_os_hint_consistency_test_with_local_program_objects to main-v0.14.0 June 8, 2025 10:30
@dorimedini-starkware dorimedini-starkware force-pushed the 06-05-test_starknet_os_deprecated_syscall_hint_consistency_test branch from e534e35 to 015c853 Compare June 8, 2025 10:30
@graphite-app
Copy link

graphite-app bot commented Jun 8, 2025

Merge activity

  • Jun 8, 10:30 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Jun 8, 2025
Merged via the queue into main-v0.14.0 with commit 61946a6 Jun 8, 2025
11 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 10, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants