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

make ProgramJson serializable #1286

Closed
wants to merge 3 commits into from
Closed

Conversation

tarrencev
Copy link

Resolves #1183

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

I'm not 100% convinced this is the right solution. That said, I'm willing to merge something like this mostly because it's harmless for the most part. ProgramJson is out of the hot path so it's less likely to change and break the serialization format.

That said, we need tests for this, at the very least unit tests but preferably integration tests too to see how the feature is used, etc.

Cargo.lock Outdated Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Outdated Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Show resolved Hide resolved
vm/src/serde/deserialize_program.rs Outdated Show resolved Hide resolved
@kariy
Copy link
Contributor

kariy commented Jun 28, 2023

That said, we need tests for this, at the very least unit tests but preferably integration tests too to see how the feature is used, etc.

Not sure how to write a meaningful test for this, we only need it to convert Program into bytes, because we exclusively use it to marshal between contract types coming from different crates...

This is our use case for it:
https://github.com/dojoengine/dojo/blob/86f448c38cb21d7feb52d89cf48aea3b73648739/crates/katana/rpc/src/utils/contract.rs#L26

@Oppen
Copy link
Contributor

Oppen commented Jun 28, 2023

Not sure how to write a meaningful test for this

I'd be happy enough with something with a logic like the following:

  1. Write a JSON string that looks like what you expect.
  2. Deserialize it into ProgramJson.
  3. Serialize the result.
  4. Deserialize again into ProgramJson.
  5. Check both instances are equal.

The logic seems convoluted, but it'll save you from considering all the irrelevant differences that might come from formatting or field ordering if comparing the raw strings, while ensuring future changes remain compatible with that text representation.
Do note you can't rely on a hash of this data here to match between two different serializations, and that should probably be documented somewhere. That said, the team still needs to define a good policy regarding docs so I'm not gonna be pressing that yet.

we only need it to convert Program into bytes, because we exclusively use it to marshal between contract types coming from different crates...

All API surface needs to consider a bit more than the special cases that point out a need, as it always come with some maintenance baggage. I don't intend to be a PITA, I'm just trying to make sure we don't create problems for downstream users and collaborators (including you) in the future. Serialization format is part of that.

Copy link
Contributor

@Oppen Oppen left a comment

Choose a reason for hiding this comment

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

Hey, sorry I didn't notice the new changes!
It looks ready to merge except for a last suggestion.

attributes: program.shared_program_data.error_message_attributes.clone(),
debug_info: program
impl From<Program> for ProgramJson {
fn from(program: Program) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's take a reference here. Since we're already cloning the contents there's no need to drop the old instance.

@pefontana
Copy link
Collaborator

This implementation is done here #1458
Thanks @tarrencev, for the contribution, we used your commits!

@pefontana pefontana closed this Oct 17, 2023
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.

Program serialization
6 participants