feat(starknet_os): add EndpointArg and support parameters passed by value

[amosStarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[amosStarkware]

• feat(starknet_os): add EndpointArg and support parameters passed by value #5024 👈 (View in Graphite)
• main

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

[amosStarkware]

+reviewer:@Yoni-Starkware

Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @Yoni-Starkware)

[dorimedini-starkware]

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Yoni-Starkware)

---

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

use cairo_vm::types::relocatable::{MaybeRelocatable, Relocatable};
use cairo_vm::vm::runners::cairo_runner::{CairoArg, CairoRunner};
use cairo_vm::Felt252;

Suggestion:

starknet_types_core::felt::Felt;

---

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

impl From<i32> for EndpointArg {
    fn from(other: i32) -> Self {

Suggestion:

value: i32

---

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

impl From<i32> for EndpointArg {
    fn from(other: i32) -> Self {
        EndpointArg::Value(ValueArg::Single(other.into()))

Suggestion:

Self::Value

---

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

    Value(ValueArg),
    Pointer(PointerArg),
}

define the enum above the From<i32> implementation

Code quote:

impl From<i32> for EndpointArg {
    fn from(other: i32) -> Self {
        EndpointArg::Value(ValueArg::Single(other.into()))
    }
}

#[derive(Clone, Debug)]
pub enum EndpointArg {
    Value(ValueArg),
    Pointer(PointerArg),
}

---

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

}

impl EndpointArg {

doesn't this work?

Suggestion:

impl From<&EndpointArg> for Vec<CairoArg> {

---

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

                ValueArg::Composed(endpoint_args) => {
                    endpoint_args.iter().flat_map(Self::to_cairo_arg_vec).collect()
                }

if I have

struct P { x: felt, y: felt }
struct NP { n: felt, p: P* }

won't this iteration (on an input of type NP) create a vector for n? I understand that the P* field should be "vectorized" but why the felt field?

Code quote:

                ValueArg::Composed(endpoint_args) => {
                    endpoint_args.iter().flat_map(Self::to_cairo_arg_vec).collect()
                }

---

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

                    endpoint_args.iter().flat_map(Self::to_cairo_arg_vec).collect(),
                )],
            },

I prefer full destructuring syntax, as opposed to nested matches.
non-blocking

Suggestion:

            EndpointArg::Value(ValueArg::Single(felt)) => {
                vec![CairoArg::Single(MaybeRelocatable::Int(*felt))]
            }
            EndpointArg::Value(ValueArg::Array(felts)) => felts
                .iter()
                .map(|felt| CairoArg::Single(MaybeRelocatable::Int(*felt)))
                .collect(),
            EndpointArg::Value(ValueArg::Composed(endpoint_args)) => {
                endpoint_args.iter().flat_map(Self::to_cairo_arg_vec).collect()
            }
            EndpointArg::Pointer(PointerArg::Array(felts)) => vec![CairoArg::Array(
                felts.iter().map(|felt| MaybeRelocatable::Int(*felt)).collect(),
            )],
            EndpointArg::Pointer(PointerArg::Composed(endpoint_args)) => vec![CairoArg::Composed(
                endpoint_args.iter().flat_map(Self::to_cairo_arg_vec).collect(),
            )],

---

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

    Array(Vec<Felt252>),
    Composed(Vec<EndpointArg>),
}

1. aren't there equivalent types defined on the VM that you can use?
2. please move to the top (above the enum that uses them)

Code quote:

/// An arg passed by value (i.e., a felt, tuple, named tuple or struct).
#[derive(Clone, Debug)]
pub enum ValueArg {
    Single(Felt252),
    Array(Vec<Felt252>),
    Composed(Vec<EndpointArg>),
}

/// An arg passed as a pointer. i.e., a pointer to a felt, tuple, named tuple or struct, or a
/// pointer to a pointer.
#[derive(Clone, Debug)]
pub enum PointerArg {
    Array(Vec<Felt252>),
    Composed(Vec<EndpointArg>),
}

---

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

    if expected_arg_is_felt != actual_arg_is_felt
        || expected_arg_is_pointer != actual_arg_is_pointer
        || expected_arg_is_struct_or_tuple != actual_arg_is_struct_or_tuple

you introduce some redundancy in booleans here... if the first two pairs are equal, then the third pair is equal by definition.
it's nice to keep for clarity though

Code quote:

    if expected_arg_is_felt != actual_arg_is_felt
        || expected_arg_is_pointer != actual_arg_is_pointer
        || expected_arg_is_struct_or_tuple != actual_arg_is_struct_or_tuple

[amosStarkware]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

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

Previously, dorimedini-starkware wrote…

define the enum above the From<i32> implementation

Done.

---

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

Previously, dorimedini-starkware wrote…

doesn't this work?

It may, but IMO it's less readable

---

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

Previously, dorimedini-starkware wrote…

if I have

struct P { x: felt, y: felt }
struct NP { n: felt, p: P* }

won't this iteration (on an input of type NP) create a vector for n? I understand that the P* field should be "vectorized" but why the felt field?

to_cairo_arg_vec(), when called on n, will return vec![CairoArg::Single(MaybeRelocatable::Int(_))] (the vec will be removed when flattened).
when called on p, it will return vec![CairoArg::Array(MaybeRelocatable::Int(felt), MaybeRelocatable::Int(felt))] (the vec will be removed when flattened).
does that answer your question? we can huddle if not

---

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

Previously, dorimedini-starkware wrote…

I prefer full destructuring syntax, as opposed to nested matches.
non-blocking

I think I like nested matches better

---

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

Previously, dorimedini-starkware wrote…

1. aren't there equivalent types defined on the VM that you can use?
2. please move to the top (above the enum that uses them)

1. not that I know of - the VM uses CairoArgs.
   The VM's entrypoint runner assumes CairoArg::Single is a felt, CairoArg::Array & CairoArg::Composed are pointers, and it doesn't provide an option to pass parameters by value.
   Also, CairoArg accepts both felts and relocatables. but we never expect a relocatable to be passed to the endpoint, so ValueArg & PointerArg only accept felts
2. They use EndpointArg too

---

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

Previously, dorimedini-starkware wrote…

you introduce some redundancy in booleans here... if the first two pairs are equal, then the third pair is equal by definition.
it's nice to keep for clarity though

Right, I didn't notice

---

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

use cairo_vm::types::relocatable::{MaybeRelocatable, Relocatable};
use cairo_vm::vm::runners::cairo_runner::{CairoArg, CairoRunner};
use cairo_vm::Felt252;

Done.

---

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

impl From<i32> for EndpointArg {
    fn from(other: i32) -> Self {

Done.

---

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

impl From<i32> for EndpointArg {
    fn from(other: i32) -> Self {
        EndpointArg::Value(ValueArg::Single(other.into()))

Done.

[amosStarkware]

Reviewable status: 5 of 6 files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware and @Yoni-Starkware)

---

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

Previously, amosStarkware wrote…

1. not that I know of - the VM uses CairoArgs.
   The VM's entrypoint runner assumes CairoArg::Single is a felt, CairoArg::Array & CairoArg::Composed are pointers, and it doesn't provide an option to pass parameters by value.
   Also, CairoArg accepts both felts and relocatables. but we never expect a relocatable to be passed to the endpoint, so ValueArg & PointerArg only accept felts
2. They use EndpointArg too

• assumes CairoArg::Single(MaybeRelocatable::Int(_)) is a Felt.

[dorimedini-starkware]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @amosStarkware and @Yoni-Starkware)

---

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

Previously, amosStarkware wrote…

to_cairo_arg_vec(), when called on n, will return vec![CairoArg::Single(MaybeRelocatable::Int(_))] (the vec will be removed when flattened).
when called on p, it will return vec![CairoArg::Array(MaybeRelocatable::Int(felt), MaybeRelocatable::Int(felt))] (the vec will be removed when flattened).
does that answer your question? we can huddle if not

ah, the flatten removes stuff... got it, thanks

---

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

Previously, amosStarkware wrote…

• assumes CairoArg::Single(MaybeRelocatable::Int(_)) is a Felt.

2. can you move these defs to above your enum, then?

[amosStarkware]

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

---

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

Previously, dorimedini-starkware wrote…

2. can you move these defs to above your enum, then?

not sure why it matters (enum uses them and they use the enum), but ok

[dorimedini-starkware]

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

---

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

Previously, amosStarkware wrote…

not sure why it matters (enum uses them and they use the enum), but ok

oh... misunderstood you remark about the recursive-type part... sorry