Skip to content
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

Why does Program do not implement serde? #1660

Open
tdelabro opened this issue Mar 12, 2024 · 18 comments
Open

Why does Program do not implement serde? #1660

tdelabro opened this issue Mar 12, 2024 · 18 comments
Labels
enhancement New feature or request

Comments

@tdelabro
Copy link
Contributor

tdelabro commented Mar 12, 2024

In vm/src/types/program.rs:

    pub fn serialize(&self) -> Result<Vec<u8>, ProgramError> {
        let program_serializer: ProgramSerializer = ProgramSerializer::from(self);
        let bytes: Vec<u8> = serde_json::to_vec(&program_serializer)?;
        Ok(bytes)
    }

    pub fn deserialize(
        program_serializer_bytes: &[u8],
        entrypoint: Option<&str>,
    ) -> Result<Program, ProgramError> {
        let program_serializer: ProgramSerializer =
            serde_json::from_slice(program_serializer_bytes)?;
        let program_json = ProgramJson::from(program_serializer);
        let program = parse_program_json(program_json, entrypoint)?;
        Ok(program)
    }

Program does not implement the traits Serialize/Deserialize, but instead those two methods who do the same thing, only in a way that is not compatible with the serde crate.
Why? I break upstream usage of the traits. If I have a struct that contains a Program (like blockifier::ContractClassV1Inner ), I cannot derive the correct trait onto it.

Why not just implement the trait?

@tdelabro tdelabro added the enhancement New feature or request label Mar 12, 2024
@pefontana
Copy link
Collaborator

Hi @tdelabro !
We've utilized the ProgramSerializer struct for serialization and deserialization of the Program struct, allowing for flexibility in future modifications. We're open to reconsidering this approach. Are you looking for the Program to specifically implement that trait, or is this just a preliminary inquiry?

@tdelabro
Copy link
Contributor Author

tdelabro commented Mar 22, 2024

Hi @pefontana,

I saw the ProgramSerializer and it's name let me understand that it may be related to my needs, but there is no doc on what it does and how to use it.

So, what we ended up doing is the following:
https://github.com/bidzyyys/blockifier/blob/feature/scale-codec/crates/blockifier/src/execution/contract_class.rs#L654-L681
and
https://github.com/bidzyyys/blockifier/blob/feature/scale-codec/crates/blockifier/src/execution/contract_class.rs#L145-L150

Adding our own custom de/serialization logic where we need it in the logic that uses your type, so that we can still implement Serialize and Deserialize on our type.

Are you looking for the Program to specifically implement that trait, or is this just a preliminary inquiry?

Yes, the Program type should implement Serialize and Deserialize so that lib users don't have to guess or hack their way through it.

@tdelabro
Copy link
Contributor Author

@pefontana any news on this issue?

@tdelabro
Copy link
Contributor Author

This is becoming a problem in other repositories: starkware-libs/blockifier#1937

@Oppen
Copy link
Contributor

Oppen commented Jun 3, 2024

I saw the ProgramSerializer and it's name let me understand that it may be related to my needs, but there is no doc on what it does and how to use it.

So the real issue is that this is not properly documented, am I correct?

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 5, 2024

So the real issue is that this is not properly documented, am I correct?

This is one thing.

The other is that it prevents anyone downstream from deriving Serde on their types that contain Program, which is a huge pain.
I see no reason why the trait would not be implemented, but maybe I'm missing something

@Oppen
Copy link
Contributor

Oppen commented Jun 5, 2024

So the real issue is that this is not properly documented, am I correct?

This is one thing.

The other is that it prevents anyone downstream from deriving Serde on their types that contain Program, which is a huge pain. I see no reason why the trait would not be implemented, but maybe I'm missing something

I believe it can be implemented, what we can not do is derive it and call it a day, as it implicitly converts private interfaces into public ones (specifically, derive(Serialize, Deserialize) forces JSON layout and in-memory layout to match. I'll try to look a bit into it.

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 6, 2024

We want serde implemented on Program so types that contains it can derive it on themselves without having to ask any question

@fmoletta
Copy link
Contributor

fmoletta commented Jun 7, 2024

The main reason for not implementing serde on Program is that we don't have a single deserialization implementation for Program itself.
When we deserialize a compiled cairo 0 program, using either Program::from_bytes or Program::from_file we convert the compiled program data to the format used by Program, losing the original information in the process.
When we then serialize Program, via serialize, we use a ProgramSerializer struct which is not equivalent to the original compiled cairo program.
We can also deserialize this ProgramSerializer via deserialize, which will net us the same Program struct we serialized via serialize.
The problem with this is that if we were to implement Deserialize relying in the compiled cairo program deserialization (aka Program::from_bytes), Serialize & Deserialize wouldn't be compatible, as deserializing a serialized program would fail.
If we were to choose the other route, and implement Deserialize relying on the ProgramSerializer deserialization, then Serialize & Deserialize would be compatible, but it wouldn't be useful in your case, as the deserialization would not work for cairo programs generated by the cairo 0 compiler

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 8, 2024

Thanks, @fmoletta for expressing clearly the issue here.
I think there is a straightforward solution here: creating different Struc.
You have your executable program, which impl Serde.
You have your DeprecatedProgram (or Cairo0Program, or whatever name), which impl Serde and into_executable_program.

Now if you have a Program, you can de/ser it freely and if you are dealing with a cairo0 file, you deserialize it into your intermediary struct that you then use to build an executable Program.

Note that you still won't be able to go from a Program to a Cairo0Program. Only the other way around.

I have understood the situation correctly and is this a viable answer to the problem?

@fmoletta
Copy link
Contributor

fmoletta commented Jun 10, 2024

Thanks, @fmoletta for expressing clearly the issue here. I think there is a straightforward solution here: creating different Struc. You have your executable program, which impl Serde. You have your DeprecatedProgram (or Cairo0Program, or whatever name), which impl Serde and into_executable_program.

Now if you have a Program, you can de/ser it freely and if you are dealing with a cairo0 file, you deserialize it into your intermediary struct that you then use to build an executable Program.

Note that you still won't be able to go from a Program to a Cairo0Program. Only the other way around.

I have understood the situation correctly and is this a viable answer to the problem?

If I understand correctly this would involve creating a DeprecatedProgram struct that has the exact same format as the compiled json file so that we can deserialize and then serialize it without losing information and then converting that to a Program. This would make our deserialization step much slower as we would need to first parse the file and then parse it again to deserialize fields such as the references in the reference manager which come in string format.

@Oppen
Copy link
Contributor

Oppen commented Jun 10, 2024

I think the idea allows for us (cairo-vm-cli) to just use a custom deserialization that goes directly to Program. The idea would be to additionally have this Cairo0Program that can be converted to Program but also embedded into other projects' structures and serialized and deserialized without information loss. Is this correct @tdelabro?

@tdelabro
Copy link
Contributor Author

Another weird thing:
The ProgramJson contains the following fields:

#[derive(Serialize, Deserialize)]
pub struct Reference {
    ...
    #[serde(deserialize_with = "deserialize_value_address")]
    #[serde(rename(deserialize = "value"))]
    pub value_address: ValueAddress,
}

Where the struct impl both Serialize and Deserialize, but won't deserialized in the same way as it serialized. The struct
Making it unusable.

In the same idea both serde::deserialize::ProgramJson and serde::serialize::ProgramSerializer implement both ser and de.
It makes absolutely no sense.

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 14, 2024

My input would be to:

  • drop the whole custom deserializer thing you have
  • define a struct for each outside world struct you want to be able to load (DeprecatedProgram, Program) (or v0, v1) (or legacy, idk)
  • implement a two-way serde for those (you should be able to call serialize then deserialize without losing data)
  • impl From<ThoseClassesYouJustCreated> for ExecutableProgram (ExecutableProgram being what is currently named Program)

Then if someone still needs it for some reason, we can easily derive Serde onto the ExecutableProgram while making it clear that this is not the format outputted by the compiler.

@fmoletta
Copy link
Contributor

I think structs containing a 1 to 1 version of the compiled cairo programs should be standarized (like contract classes in starknet-api) so users don't have to depend on this crate for it

@tdelabro
Copy link
Contributor Author

@orizi ⬆️

@Eikix
Copy link
Contributor

Eikix commented Jun 28, 2024

@orizi ⬆️

we would also like to be able to stop using forks of blockifier, they're very hard to maintain over time

@tdelabro
Copy link
Contributor Author

@pefontana @Oppen @fmoletta Did you decide on a design? What is the ETA?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants