Skip to content

stavbe: change AirPrivateInputSerializable fields to public #1900

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

Stavbe
Copy link
Collaborator

@Stavbe Stavbe commented Dec 24, 2024

Change AirPrivateInputSerializable fields to be public, so I can use this struct.


This change is Reviewable

@Stavbe Stavbe force-pushed the stavbe/air-private-input-fields-to-pub branch from 4205074 to 60912a7 Compare December 24, 2024 08:55
@gabrielbosio
Copy link
Collaborator

gabrielbosio commented Jan 3, 2025

Hi @Stavbe! You can convert AirPrivateInputSerializable into an AirPrivateInput and then get each builtin, for example to get Pedersen:

fn get_pedersen(serializable: AirPrivateInputSerializable) -> Some(Vec<PrivateInput>>) {
  let air_private_input: AirPrivateInput = serializable.into();
  air_private_input.0.get(&BuiltinName::pedersen)
}

With this, there's no need to make AirPrivateInputSerializable fields public.

@Stavbe
Copy link
Collaborator Author

Stavbe commented Jan 5, 2025 via email

@gabrielbosio
Copy link
Collaborator

gabrielbosio commented Jan 7, 2025

Ok, I'm trying to get a bit more of context here so what's the use case? Because when calling AirPrivateInput::to_serializable one passes both trace and mem paths:

pub fn to_serializable(
&self,
trace_path: String,
memory_path: String,
) -> AirPrivateInputSerializable {

@Stavbe
Copy link
Collaborator Author

Stavbe commented Jan 23, 2025 via email

Copy link
Collaborator Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Sorry, I missed that response.
Yes, the JSON file (which was created after calling to_serializable) contains the mem and trace.
Now, when trying to read it (private.json), I need to define a new struct to use those fields, which is redundant.
For example, I read pub.json into the pub struct PublicInput<'a>.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @Oppen, and @pefontana)

Copy link
Contributor

@JulianGCalderon JulianGCalderon left a comment

Choose a reason for hiding this comment

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

Hi @Stavbe! Would exposing only trace_path and memory_pathsuffice? Or maybe we could add getter methods (i.e get_trace_path()).

@Stavbe Stavbe force-pushed the stavbe/air-private-input-fields-to-pub branch from 60912a7 to 3ad0c75 Compare March 11, 2025 15:27
@Stavbe Stavbe requested review from yuvalsw and noaov1 as code owners March 11, 2025 15:27
Copy link
Collaborator Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Yes. that could work. Fixed :)

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @noaov1, @Oppen, @pefontana, and @yuvalsw)

@Stavbe Stavbe force-pushed the stavbe/air-private-input-fields-to-pub branch from 3ad0c75 to d9d8e52 Compare March 11, 2025 16:48
@Stavbe Stavbe force-pushed the stavbe/air-private-input-fields-to-pub branch from d9d8e52 to 61a1c75 Compare March 11, 2025 16:50
Copy link
Collaborator

@yuvalsw yuvalsw left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 2 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @noaov1, @Oppen, and @pefontana)


-- commits line 5 at r3:
needs rebase?

Code quote:

New commits in r2 on 11/03/2025 at 18:49, probably rebased from r1:
- 61a1c75: stavbe: change AirPrivateInputSerializable fields to public

New commits in r3 on 16/03/2025 at 15:52:
- fda1f4c: Merge branch 'main' into stavbe/air-private-input-fields-to-pub

@Stavbe Stavbe force-pushed the stavbe/air-private-input-fields-to-pub branch from fda1f4c to 61a1c75 Compare March 16, 2025 15:09
@Stavbe
Copy link
Collaborator Author

Stavbe commented Mar 16, 2025

Hey :)
Why am I not able to merge ?

Copy link
Collaborator Author

@Stavbe Stavbe left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @fmoletta, @gabrielbosio, @igaray, @juanbono, @noaov1, @Oppen, and @pefontana)

Comment on lines +15 to +16
pub trace_path: String,
pub memory_path: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the idea is to read these fields and to not write them, I think it would be better to add getters for these fields instead of making them public

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It wouldn't work because I read it from json file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not completely sure how reading the struct from a json file restricts implementing getters instead of making these fields public.
Let me rephrase my suggestion just in case. For example, instead of accessing these fields like this:

let trace_path = air_private_input_serializable.trace_path;
let memory_path = air_private_input_serializable.memory_path;

Shouldn't this also work?

let trace_path = air_private_input_serializable.get_trace_path();
let memory_path = air_private_input_serializable.get_memory_path();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before being able to call the getter, you need to construct the sturct from the json:
fn deserialize_inputs<'a>( public_input_string: &'a str, private_input_string: &'a str, ) -> Result<(PublicInput<'a>, PrivateInput), VmImportError> { { Ok(( sonic_rs::from_str(public_input_string)?, sonic_rs::from_str(private_input_string)?, )) } #[cfg(not(feature = "std"))] { Ok(( serde_json::from_str(public_input_string)?, serde_json::from_str(private_input_string)?, )) }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, but if you make trace_path and memory_path public, wouldn't you also need to construct the struct from the json?

@Stavbe Stavbe closed this Mar 30, 2025
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.

4 participants