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

Remove custom Serde implementation #44

Merged
merged 9 commits into from
Dec 23, 2021
Merged

Conversation

daxpedda
Copy link
Contributor

Based on #42 and #43 we don't really need a custom implementation for Serde anymore.

Advantages:

  • Less code to maintain on our side.
  • No weird where bounds for consumers using Serde instead of our de/serialize functions.
  • Better Serde implementation, instead of calling our de/serialize function, which does manual de/serialization.

Disadvantages:

  • Cargo's weak dependency feature isn't stable yet, consumers will have to manually activate the serde crate feature for the curve25519-dalek or p256 dependency. Not that big of a deal considering we aren't re-exporting them anyway, consumers will have to pull in these crates manually anyway.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 20, 2021
@daxpedda daxpedda force-pushed the serde branch 3 times, most recently from 6bc820e to 7529328 Compare December 23, 2021 01:46
@kevinlewi kevinlewi marked this pull request as ready for review December 23, 2021 20:02
@kevinlewi kevinlewi merged commit 5228474 into facebook:main Dec 23, 2021
This was referenced Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants