delete redudent struct from json.rs

[Stavbe]

This change is 

[yuvalsw]

Thanks for the cleanup!

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)

---

a discussion (no related file):
There are differences in the structs from LC's repo and the ones you deleted here in types of integers (usize in LC and u32/u64 here). This might be delicate sometimes. Did you make sure there is no scenario in which this change is significant?

One thing we'll have to do is to run an e2e test before merging this PR.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 35 at r1 (raw file):

    let _span = span!(Level::INFO, "import_from_vm_output").entered();
    let pub_data_string = std::fs::read_to_string(pub_json)?;
    let pub_data: PublicInput<'_> = sonic_rs::from_str(&pub_data_string)?;

is this <'_> required?

Code quote:

PublicInput<'_>

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r1 (raw file):

type RangeCheckData = PrivateInputValue;

// Can't use AirPrivateInputSerializable since trace_path and mem_path are private.

1. better specify which (or in this case, from what repo) AirPrivateInputSerializable you refer to.
2. Not in this PR, but I think it's worth sending a PR to LC to change that

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 51 at r1 (raw file):

#[derive(Clone, Debug)]
pub struct FeltValue([u8; 32]);

The equivalent type used in LC's repo is starknet_types_core::felt::Felt which is implemented differently. Did you verify this difference is not important here?

[Stavbe]

It made your life harder, don’t deny it.

Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @yuvalsw)

---

a discussion (no related file):

Previously, yuvalsw wrote…

There are differences in the structs from LC's repo and the ones you deleted here in types of integers (usize in LC and u32/u64 here). This might be delicate sometimes. Did you make sure there is no scenario in which this change is significant?

One thing we'll have to do is to run an e2e test before merging this PR.

The data is output from the VM using the LC types. That’s why I feel it’s not risky, but even more accurate to use the original types and make a conversion if needed. There is no use for this code, so I can’t understand the intention behind creating it,
but I suspect it was part of some POC.
If you think it’s too risky, we can leave it as is.

WDYT @shaharsamocha7 ?

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r1 (raw file):

Previously, yuvalsw wrote…

1. better specify which (or in this case, from what repo) AirPrivateInputSerializable you refer to.
2. Not in this PR, but I think it's worth sending a PR to LC to change that

1. Done
2. The problem is that I’m not sure if it’s intentional, and I couldn’t find anyone who can verify that I won’t be accused of breaking the vm security.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 51 at r1 (raw file):

Previously, yuvalsw wrote…

The equivalent type used in LC's repo is starknet_types_core::felt::Felt which is implemented differently. Did you verify this difference is not important here?

Well, I didn’t find a use for this, so I can’t be 100% sure... But note that VM outputs the data using the type starknet_types_core::felt::Felt, and I noticed there are functions that can convert it to [u8; 32] in the impl of FieldElement<Stark252PrimeField>, so I don't see a good reason to keep it.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 35 at r1 (raw file):

Previously, yuvalsw wrote…

is this <'_> required?

Yes.. Clippy : error: hidden lifetime parameters in types are deprecated

[shaharsamocha7]

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 2 of 4 files reviewed, 5 unresolved discussions (waiting on @Stavbe and @yuvalsw)

---

a discussion (no related file):

Previously, Stavbe wrote…

The data is output from the VM using the LC types. That’s why I feel it’s not risky, but even more accurate to use the original types and make a conversion if needed. There is no use for this code, so I can’t understand the intention behind creating it,
but I suspect it was part of some POC.
If you think it’s too risky, we can leave it as is.

WDYT @shaharsamocha7 ?

I think that first we need to decide if we want to keep this flow or discard it.
If yes, we need to make this code support all the builtins and add tests.
If not, why not remove all of it?

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r2 (raw file):

type RangeCheckData = PrivateInputValue;

// Can't use cairo_vm::air_private_input::AirPrivateInputSerializable since trace_path and mem_path

Note: you can use a link to code with ``

Code quote:

cairo_vm::air_private_input::AirPrivateInputSerializable

[yuvalsw]

I neither confirm nor deny

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @Stavbe)

---

a discussion (no related file):

Previously, shaharsamocha7 wrote…

I think that first we need to decide if we want to keep this flow or discard it.
If yes, we need to make this code support all the builtins and add tests.
If not, why not remove all of it?

Stav - makes sense, thanks.
Shahar - what flow do you refer to? Anyway, this PR only removes unused stuff and aligns things better with the VM - is there a downside to it? If we later can remove more code - we will.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r1 (raw file):

Previously, Stavbe wrote…

1. Done
2. The problem is that I’m not sure if it’s intentional, and I couldn’t find anyone who can verify that I won’t be accused of breaking the vm security.

2. pub only changes the access control to these fields. The vm itself will still have the exact same logic. It can only "break" something if later used somewhere where it shouldn't have been. Anyway, if it's super important to LC, they can say it in the review. In that case, we probably also want to know why (maybe it applies to us too?). But I really think it's not.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 51 at r1 (raw file):

Previously, Stavbe wrote…

Well, I didn’t find a use for this, so I can’t be 100% sure... But note that VM outputs the data using the type starknet_types_core::felt::Felt, and I noticed there are functions that can convert it to [u8; 32] in the impl of FieldElement<Stark252PrimeField>, so I don't see a good reason to keep it.

OK, sounds right. Can you point to this impl though? I couldn't find it.

[Stavbe]

• unify adapter functionality #274
• support builtins in the adapter #269
• delete redudent struct from json.rs #268 👈 (View in Graphite)
• main

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

[Stavbe]

Reviewable status: 2 of 4 files reviewed, 4 unresolved discussions (waiting on @shaharsamocha7 and @yuvalsw)

---

a discussion (no related file):

Previously, yuvalsw wrote…

Stav - makes sense, thanks.
Shahar - what flow do you refer to? Anyway, this PR only removes unused stuff and aligns things better with the VM - is there a downside to it? If we later can remove more code - we will.

He meant that if we decide to clean it, we need to remove all the unnecessary fields. I left only the two private fields, added a TODO to delete them, and opened a PR in the LC repository.

Regarding testing test_read_from_small_files and test_read_from_large_files pass (both use the import_from_vm_output function that changed).

To my understanding, the Cairo input will only need the memory section addresses of the builtins - so the changes of the other types won't affect anything.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r1 (raw file):

Previously, yuvalsw wrote…

2. pub only changes the access control to these fields. The vm itself will still have the exact same logic. It can only "break" something if later used somewhere where it shouldn't have been. Anyway, if it's super important to LC, they can say it in the review. In that case, we probably also want to know why (maybe it applies to us too?). But I really think it's not.

Yes. working on it, added a todo

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 51 at r1 (raw file):

Previously, yuvalsw wrote…

OK, sounds right. Can you point to this impl though? I couldn't find it.

to_bites_be() in /starknet-types-core-0.1.7/src/felt/mod.rs

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 7 at r2 (raw file):

Previously, shaharsamocha7 wrote…

Note: you can use a link to code with ``

Done.

[yuvalsw]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shaharsamocha7)

[shaharsamocha7]

Reviewed 1 of 4 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Stavbe)

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 37 at r3 (raw file):

    let _span = span!(Level::INFO, "import_from_vm_output").entered();
    let pub_data_string = std::fs::read_to_string(pub_json)?;
    let pub_data: PublicInput<'_> = sonic_rs::from_str(&pub_data_string)?;

Revert this change

Code quote:

    let pub_data_string = std::fs::read_to_string(pub_json)?;
    let pub_data: PublicInput<'_> = sonic_rs::from_str(&pub_data_string)?;

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 3 at r3 (raw file):

use serde::{Deserialize, Serialize};
// TODO(Stav): delete this struct after making those fieldes public in
// `vm::air_private_input::AirPrivateInputSerializable.

I think you should also add a doc to this struct, besides for the TODO
Suggested down

Suggestion:

// Struct to store Cairo private input.
// Replicated from `cairo_vm::air_private_input::AirPrivateInputSerializable`
// TODO(Stav): Replace with original struct once fields are public.

[Stavbe]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7 and @yuvalsw)

---

stwo_cairo_prover/crates/prover/src/input/vm_import/json.rs line 3 at r3 (raw file):

Previously, shaharsamocha7 wrote…

I think you should also add a doc to this struct, besides for the TODO
Suggested down

Done.

---

stwo_cairo_prover/crates/prover/src/input/vm_import/mod.rs line 37 at r3 (raw file):

Previously, shaharsamocha7 wrote…

Revert this change

i am getting a life time issue: "creates a temporary value which is freed while still in use"

[shaharsamocha7]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Stavbe)