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

Add array serialization impls #371

Merged
merged 10 commits into from
Jan 7, 2022
Merged

Conversation

ryanleh
Copy link
Contributor

@ryanleh ryanleh commented Dec 29, 2021

Description

This PR impls CanonicalSerialize and CanonicalDeserialize for arrays of up to size 32. This is useful for deriving these traits on structs which contain arrays.

I put Default and Copy bounds on CanonicalDeserialize in order to initialize the initial empty array. We could get rid of these bounds and use an Option<T>, but I believe this necessitates heap allocation in order to unwrap the Option<T> for the final result which I thought best to avoid. Let me know what y'all think is best.

I'll complete the other TODOs once I get some form of approval from maintainers to move forward (not sure who to ask for review 😁 )


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code (NA)
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@ryanleh ryanleh changed the title Add array impl Add array serialization impls Dec 29, 2021
@mmagician
Copy link
Member

@ryanleh I recently started re-working an oldish PR to replace some parts of the code with const generics: I have a draft of the changes to bigint crate here: #372.
Perhaps it would make sense for this PR to also utilise const generics? I believe it would be analogous to this impl.

Otherwise Copy and Default trait bounds sound reasonable. I think most of the types from algebra that would make sense to put in an array have these implemented.

@ryanleh
Copy link
Contributor Author

ryanleh commented Jan 4, 2022

@mmagician Ah, had totally missed that const generics had stabilized!! 😁 That definitely makes more sense -- I updated the PR appropriately.

}
}

impl<T: CanonicalDeserialize + Default + Copy, const N: usize> CanonicalDeserialize for [T; N] {
Copy link
Member

Choose a reason for hiding this comment

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

I believe we can use new array methods to eliminate the Default + Copy bounds:

Ok([(); N].map(|_| T::deserialize(&mut reader).unwrap()))

There is a downside that this panics immediately if there's an error, but that's not necessarily a bad thing. This might be the best we can do until this feature is stabilized.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome! I changed de-serialization to do this, removed the extra bounds, and added a TODO to update things once the feature is stabilized.

@ryanleh
Copy link
Contributor Author

ryanleh commented Jan 7, 2022

For some reason the CanonicalSerialize impl is causing serialization tests for BTreeMap, BTreeSet, Tuple, Option, and Rc to panic..? Not sure what's going on lol I'll take a look in a bit.

@Pratyush
Copy link
Member

Pratyush commented Jan 7, 2022

For some reason the CanonicalSerialize impl is causing serialization tests for BTreeMap, BTreeSet, Tuple, Option, and Rc to panic..? Not sure what's going on lol I'll take a look in a bit.

Should be fixed in the latest commit

@Pratyush Pratyush merged commit edea6bb into arkworks-rs:master Jan 7, 2022
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