Skip to content

Conversation

@Idanh-starkware
Copy link
Contributor

@Idanh-starkware Idanh-starkware commented Aug 20, 2025

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

This change is Reviewable

Copy link
Contributor Author

Idanh-starkware commented Aug 20, 2025

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

@Idanh-starkware Idanh-starkware mentioned this pull request Aug 20, 2025
9 tasks
@Idanh-starkware Idanh-starkware marked this pull request as ready for review August 20, 2025 08:04
@Idanh-starkware Idanh-starkware self-assigned this Aug 20, 2025
@Idanh-starkware Idanh-starkware force-pushed the idanh/enableing_execute_task_tests branch from e577a4e to e061dd6 Compare August 20, 2025 08:05
Copy link
Contributor

@OmriEshhar1 OmriEshhar1 left a comment

Choose a reason for hiding this comment

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

@OmriEshhar1 reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @YairVaknin-starkware)


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 584 at r1 (raw file):

#[cfg(test)]
mod tests {
    //use std::sync::{Arc, ReentrantLock};

remove commented lines (here and below) ?

Code quote:

 //use std::sync::{Arc, ReentrantLock};

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 588 at r1 (raw file):

    use crate::hints::codes::*;
    use crate::hints::types::{
        /* BootloaderConfig, */ Cairo0Executable, /* SimpleBootloaderInput, */ Task,

remove commented out parts?

Code quote:

 /* BootloaderConfig, */ Cairo0Executable, /* SimpleBootloaderInput, */ Task,

crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 592 at r1 (raw file):

    use crate::test_utils::{
        get_hint_codes_at_pc,
        /* prepare_ids_data_for_test, */ prepare_non_continuous_ids_data_for_test,

remove commented out parts?

Code quote:

 /* prepare_ids_data_for_test, */ prepare_non_continuous_ids_data_for_test,

crates/cairo-program-runner-lib/src/test_utils.rs line 14 at r1 (raw file):

/// Test helper: Create a HashMap of HintReferences for testing.
pub fn prepare_refrences_for_test(num: i32) -> std::collections::HashMap<usize, HintReference> {

I see that you removed the tests (long) documentation when you moved them to test_utils.
Is there a reason for it?


crates/cairo-program-runner-lib/src/hints/program_loader.rs line 98 at r1 (raw file):

        let builtins = &program.builtins;
        let n_builtins = builtins.len();
        println!(

remove the println-s (here and elsewhere) ?

@Idanh-starkware Idanh-starkware force-pushed the idanh/enableing_execute_task_tests branch from e061dd6 to 4b91e6d Compare August 20, 2025 12:36
@Idanh-starkware
Copy link
Contributor Author

crates/cairo-program-runner-lib/src/test_utils.rs line 14 at r1 (raw file):

Previously, OmriEshhar1 wrote…

I see that you removed the tests (long) documentation when you moved them to test_utils.
Is there a reason for it?

I blame gpt

@Idanh-starkware Idanh-starkware force-pushed the idanh/enableing_execute_task_tests branch from 4b91e6d to 722e685 Compare August 20, 2025 12:50
Copy link
Contributor Author

@Idanh-starkware Idanh-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: 5 of 8 files reviewed, 5 unresolved discussions (waiting on @OmriEshhar1 and @YairVaknin-starkware)


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 584 at r1 (raw file):

Previously, OmriEshhar1 wrote…

remove commented lines (here and below) ?

Done.


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 588 at r1 (raw file):

Previously, OmriEshhar1 wrote…

remove commented out parts?

Done.


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 592 at r1 (raw file):

Previously, OmriEshhar1 wrote…

remove commented out parts?

Done.


crates/cairo-program-runner-lib/src/hints/program_loader.rs line 98 at r1 (raw file):

Previously, OmriEshhar1 wrote…

remove the println-s (here and elsewhere) ?

Done.

@Idanh-starkware Idanh-starkware mentioned this pull request Aug 20, 2025
9 tasks
Copy link
Contributor

@OmriEshhar1 OmriEshhar1 left a comment

Choose a reason for hiding this comment

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

@OmriEshhar1 reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @YairVaknin-starkware)


a discussion (no related file):
I see that some documentation you do above the function declaration with /// and some inside the function with //.
Can you align it?
(I guess that /// is better because it creates automatic documentation or something, isn't it?)


crates/cairo-program-runner-lib/src/test_utils.rs line 87 at r2 (raw file):

/// The `task` is inserted into the execution scopes. `load_program_hint` should succeed after this
/// function.
pub fn prepare_vm_for_load_program_loading_test(

it looks like you always call this function with the same task (fibonacci_task).
so maybe just make it hard-coded in the function?


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 389 at r2 (raw file):

/// If the program is Cairo0 or Cairo1, it processes the program's hints and constants.
/// If the program is a CairoPIE, it loads the PIE into memory and sets up the necessary pointers.
/// It also prepares the output runner data and enters a new scope with the task locals

add . at the end (sorry...)


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 1020 at r2 (raw file):

                                 // Initialize the pre-execution struct to [1, 2, ..., 11].
                                 // Initialize the used builtins to {output: 30, pedersen: 31, ..., mul_mod: 40} as these are
                                 // used by the builtin_usage program.

indent left
(you can add an empty line so it doesn't move to the right automatically)

Code quote:

                                 // Allocate space for all the builtin list structs (3 x 11 felts).
                                 // The pre-execution struct starts at (1, 0), the used builtins list at (1, 11)
                                 // and the return struct at (1, 22).
                                 // Initialize the pre-execution struct to [1, 2, ..., 11].
                                 // Initialize the used builtins to {output: 30, pedersen: 31, ..., mul_mod: 40} as these are
                                 // used by the builtin_usage program.

@Idanh-starkware Idanh-starkware force-pushed the idanh/enableing_execute_task_tests branch from 722e685 to 1a731f2 Compare August 21, 2025 14:38
Copy link
Contributor Author

@Idanh-starkware Idanh-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: 7 of 8 files reviewed, 4 unresolved discussions (waiting on @OmriEshhar1 and @YairVaknin-starkware)


a discussion (no related file):

Previously, OmriEshhar1 wrote…

I see that some documentation you do above the function declaration with /// and some inside the function with //.
Can you align it?
(I guess that /// is better because it creates automatic documentation or something, isn't it?)

Done.


crates/cairo-program-runner-lib/src/test_utils.rs line 87 at r2 (raw file):

Previously, OmriEshhar1 wrote…

it looks like you always call this function with the same task (fibonacci_task).
so maybe just make it hard-coded in the function?

Done.


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 389 at r2 (raw file):

Previously, OmriEshhar1 wrote…

add . at the end (sorry...)

Done.


crates/cairo-program-runner-lib/src/hints/execute_task_hints.rs line 1020 at r2 (raw file):

Previously, OmriEshhar1 wrote…

indent left
(you can add an empty line so it doesn't move to the right automatically)

Done.

Copy link
Contributor

@OmriEshhar1 OmriEshhar1 left a comment

Choose a reason for hiding this comment

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

@OmriEshhar1 reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @YairVaknin-starkware)

@Idanh-starkware Idanh-starkware merged commit a00b69f into main Aug 25, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants