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

feature: add conditional support for serde #180

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

hackaugusto
Copy link
Contributor

Describe your changes

Adds conditional support to serde. This can be used to serialize the data as JSON to encode the mock chain data.

This depends on facebook/winterfell#209

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

@hackaugusto hackaugusto force-pushed the hacka-conditional-support-for-serde branch from 7b7b693 to 19f7a68 Compare August 8, 2023 15:10
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Left a suggestion inline. Let me know what you think.

src/hash/blake/mod.rs Outdated Show resolved Hide resolved
@hackaugusto hackaugusto force-pushed the hacka-conditional-support-for-serde branch 3 times, most recently from 7109eca to 9e53d0a Compare August 9, 2023 15:09
@hackaugusto
Copy link
Contributor Author

hackaugusto commented Aug 9, 2023

some notes:

  • I have updated the code to emit a hex encoding for the rpo and blake digests
  • I assumed this should not have additional libraries, so instead of using the hex dependency there is some simple code to do the serde of the types
  • I made the prefix 0x required for simplicity, this can easily be lifted if necessary

I now have to look how to skip encoding the inner nodes of the trees / merkle store

edit: for the time being we are serializing the whole structures, if it becomes a necessity skipping the inner nodes can be implemented

@hackaugusto hackaugusto force-pushed the hacka-conditional-support-for-serde branch from 9e53d0a to 1e77341 Compare August 10, 2023 17:53
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left question/comment inline - if you agree, we can implement these checks and merge.

src/hash/rpo/digest.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Looks good! Would be good if we can leverage the hex utilities you have introduced for midne-vm as currently have a bit of logic duplication.

impl std::error::Error for HexParseError {}

/// Parses a hex string into an array of bytes of known size.
pub fn hex_to_bytes<const N: usize>(value: &str) -> Result<[u8; N], HexParseError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a similar utility downstream in miden-vm. Would be good if we can purge that and leverage this. https://github.com/0xPolygonMiden/miden-vm/blob/105ee9e241fafeac97fbcd14861b2eaed5548e1e/assembly/src/ast/parsers/labels.rs#L77-L93

@hackaugusto hackaugusto force-pushed the hacka-conditional-support-for-serde branch from 1e77341 to 8cf5e9f Compare August 11, 2023 12:00
@hackaugusto hackaugusto merged commit d9e8523 into next Aug 11, 2023
9 checks passed
@hackaugusto hackaugusto deleted the hacka-conditional-support-for-serde branch August 11, 2023 12:03
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.

3 participants