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 Nov 23, 2025

Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

@Yoni-Starkware reviewed all commit messages.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)


crates/starknet_os/src/hints/hint_implementation/aggregator/utils.rs line 256 at r1 (raw file):

    let class_dict_ptr_start = state_diff_writer.get_class_dict_ptr();
    state_diff_writer.write_classes_changes(&state_diff.classes, vm)?;

This is written in a full output format, always. Are you going to change that? what is the motivation of passing the flag then?

Code quote:

    let state_dict_ptr_start = state_diff_writer.get_state_dict_ptr();
    state_diff_writer.write_contract_changes(&state_diff.contracts, vm)?;

    let class_dict_ptr_start = state_diff_writer.get_class_dict_ptr();
    state_diff_writer.write_classes_changes(&state_diff.classes, vm)?;

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: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @TzahiTaub and @Yoni-Starkware)


crates/starknet_os/src/hints/hint_implementation/aggregator/utils.rs line 256 at r1 (raw file):

Previously, Yoni-Starkware (Yoni) wrote…

This is written in a full output format, always. Are you going to change that? what is the motivation of passing the flag then?

used here.
IIUC this just changes the boolean felt representing the flag... the original test had this logic and I was under the impression that the OS diffs are always full-ouptut, but the aggregator output also has this flag as part of it's output (though the value is ignored...?)

@dorimedini-starkware dorimedini-starkware force-pushed the 11-23-starknet_os_allow_full_os_output_with_variable_full_output_flag branch from 8057936 to e405089 Compare December 7, 2025 10:30
@dorimedini-starkware dorimedini-starkware changed the base branch from 11-23-starknet_os_split_out_fullosoutputs_parsing_logic to main December 7, 2025 10:30
@dorimedini-starkware dorimedini-starkware force-pushed the 11-23-starknet_os_allow_full_os_output_with_variable_full_output_flag branch from e405089 to de66339 Compare December 7, 2025 14:54
Copy link
Collaborator

@Yoni-Starkware Yoni-Starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

@Yoni-Starkware reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)


crates/starknet_os/src/hints/hint_implementation/aggregator/utils.rs line 256 at r1 (raw file):

Previously, dorimedini-starkware wrote…

used here.
IIUC this just changes the boolean felt representing the flag... the original test had this logic and I was under the impression that the OS diffs are always full-ouptut, but the aggregator output also has this flag as part of it's output (though the value is ignored...?)

Okay, since it's only for tests...

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Dec 10, 2025
Merged via the queue into main with commit 79fb8cb Dec 10, 2025
18 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 11, 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