Skip to content

Conversation

@nimrod-starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor Author

nimrod-starkware commented Jul 3, 2025

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 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 47 at r1 (raw file):

#[derive(Clone, Debug, Eq, PartialEq)]
pub enum PointerArg {
    Array(Vec<MaybeRelocatable>),

it's confusing that this is MaybeRelocatable and the rest are Felts - can you make them all MaybeRelocatable?
if t his will cause conflicts in your stack - you can add a TODO and do it in a later PR

Code quote:

Array(Vec<MaybeRelocatable>),

crates/starknet_os/src/test_utils/cairo_runner.rs line 436 at r1 (raw file):

            PointerArg::Array(array) => {
                let array_pointer = vm.get_relocatable(address)?;
                let felt_array = load_sequence_of_felts(array.len(), array_pointer, vm)?;

non blocking - it's still a felt array, no need to rename

Code quote:

let felt_array = load_sequence_of_felts(array.len(), array_pointer, vm)?;

@nimrod-starkware nimrod-starkware force-pushed the nimrod/cairo_runner/pointers_to_maybe branch from 414735b to c51a852 Compare July 6, 2025 12:33
Copy link
Contributor Author

@nimrod-starkware nimrod-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, 2 unresolved discussions (waiting on @amosStarkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 47 at r1 (raw file):

Previously, amosStarkware wrote…

it's confusing that this is MaybeRelocatable and the rest are Felts - can you make them all MaybeRelocatable?
if t his will cause conflicts in your stack - you can add a TODO and do it in a later PR

WDYM?
You suggest that ValueArg::Array will also wrap Vec<MaybeRelocatable>?


crates/starknet_os/src/test_utils/cairo_runner.rs line 436 at r1 (raw file):

Previously, amosStarkware wrote…

non blocking - it's still a felt array, no need to rename

reverted.

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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 47 at r1 (raw file):

Previously, nimrod-starkware wrote…

WDYM?
You suggest that ValueArg::Array will also wrap Vec<MaybeRelocatable>?

also Single. maybe we can use CairoAg instead of ValueArg?
if we do that, maybe we also don't need PointerArg?
need to think about this

Copy link
Contributor Author

@nimrod-starkware nimrod-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/test_utils/cairo_runner.rs line 47 at r1 (raw file):

Previously, amosStarkware wrote…

also Single. maybe we can use CairoAg instead of ValueArg?
if we do that, maybe we also don't need PointerArg?
need to think about this

I'll add a todo to consider this, can i put the todo on your name?

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 @nimrod-starkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 47 at r1 (raw file):

Previously, nimrod-starkware wrote…

I'll add a todo to consider this, can i put the todo on your name?

I thought about it, and changing Single(Felt), Array(Vec<Felt>), to Single(MaybeRelocatable), Array(Vec<MaybeRelocatable>), should be enough.
can you do that / add a TODO?

@nimrod-starkware nimrod-starkware force-pushed the nimrod/cairo_runner/pointers_to_maybe branch from c51a852 to 92c0134 Compare July 7, 2025 07:00
Copy link
Contributor Author

@nimrod-starkware nimrod-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/test_utils/cairo_runner.rs line 47 at r1 (raw file):

Previously, amosStarkware wrote…

I thought about it, and changing Single(Felt), Array(Vec<Felt>), to Single(MaybeRelocatable), Array(Vec<MaybeRelocatable>), should be enough.
can you do that / add a TODO?

Done

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 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 389 at r3 (raw file):

    vm: &VirtualMachine,
) -> Result<Vec<MaybeRelocatable>, LoadReturnValueError> {
    let mut felt_array = vec![];

let mut felt_array = vec![];
may not be felt

@nimrod-starkware nimrod-starkware force-pushed the nimrod/cairo_runner/pointers_to_maybe branch from 92c0134 to fb900d4 Compare July 7, 2025 15:24
Copy link
Contributor Author

@nimrod-starkware nimrod-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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)


crates/starknet_os/src/test_utils/cairo_runner.rs line 389 at r3 (raw file):

Previously, amosStarkware wrote…

let mut felt_array = vec![];
may not be felt

Done

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 r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @nimrod-starkware)

@nimrod-starkware nimrod-starkware added this pull request to the merge queue Jul 7, 2025
Merged via the queue into main-v0.14.0 with commit 2e8c047 Jul 7, 2025
14 of 16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 9, 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