Skip to content

Re-implement serialization#201

Merged
robknight merged 9 commits intomainfrom
refactor_serialization
Apr 22, 2025
Merged

Re-implement serialization#201
robknight merged 9 commits intomainfrom
refactor_serialization

Conversation

@robknight
Copy link
Collaborator

@robknight robknight commented Apr 18, 2025

Closes #195

Re-implements serialization based on the new refactored code.

Serialization format should be pretty similar to how it was before, with some small cosmetic changes.

I've had to implement a few more custom serialization functions, in order to avoid serializing middleware data. I've commented in the places where this was necessary, so that we can be aware that those custom serialization functions will not automatically track changes to the types in the way that Serde's attribute-based serialization does.

The new version has more tests, and in particular has an end-to-end test which:

  • Serializes a SignedPod, and two quite different MainPods
  • Generates JSON Schemas for SignedPod and MainPod
  • Tests that the real SignedPod and MainPods can be validated at runtime using the schemas

@robknight robknight marked this pull request as ready for review April 18, 2025 15:25
@robknight robknight changed the title Re-implement serialization (wip) Re-implement serialization Apr 18, 2025
@robknight robknight requested a review from ed255 April 18, 2025 15:27
Copy link
Collaborator

@ed255 ed255 left a comment

Choose a reason for hiding this comment

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

LGTM! I've left two comments.
One is a small suggestion, the other is more a question/discussion.

// }
// }
impl MockSignedPod {
pub fn deserialize(id: PodId, signature: String, kvs: HashMap<Key, Value>) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this looks like a constructor with a different name :P
Will this method be used externally? If not, can we make it pub(crate)?

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 does have to be public if we want users of the library to be able to deserialize SignedPods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Won't users of the library interface the library through the frontend? And deserialize frontend::SignedPod (which internally deserializes MockSignedPod)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, sorry, you're quite right. I'll make it pub(crate).

@@ -30,10 +30,9 @@ impl TryFrom<SignedPodHelper> for SignedPod {
return Err(anyhow::anyhow!("pod_type is not Mock"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about deserializing a non-mock SignedPod?
Would this implementation have a switch here?

Same for the MainPod.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When I first wrote this, there were no non-mock SignedPods! Are the new non-mock Pods usable enough for me to implement serialization for them?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Both the non-mock SignedPod and MainPod should be ready enough to get serialization and deserialization :)
I would suggest doing that in another PR though.

@robknight robknight force-pushed the refactor_serialization branch from aefc054 to 68b004e Compare April 22, 2025 10:43
@robknight robknight merged commit bf6d8ae into main Apr 22, 2025
5 checks passed
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.

Reimplement serialization/deserialization

2 participants