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 reliance on hpke's serde implementation #311

Merged
merged 4 commits into from
Nov 27, 2023

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Nov 16, 2023

We are removing the serde_impls feature from hpke (rozbb/rust-hpke#53). This pull request cleans up some HPKE code, and moves it off of the HPKE serde impls. The serde code here is identical to hpke's.

The only thing I do not check here is that this change does not break existing databases. Since the code is identical, it would be very surprising if it did. But nonetheless, this should be checked. I would appreciate if someone could help verify this.

Let me know if you have any questions. Sorry for the hassle about removing the feature. Turns out there's lots of things people use serde for, and implementing serialization for everyone all at once is probably not the right choice for us.

@ecton
Copy link
Member

ecton commented Nov 16, 2023

No need to apologize for making a breaking change! If you look at my change log, there's a lot in this project's history 😆.

I'll make sure to test backwards compatibility before releasing an update.

Thank you so much for the PR!

@rozbb
Copy link
Contributor Author

rozbb commented Nov 16, 2023

Great, thanks for the quick response! The 0.12 release hasn't dropped yet and it might have one more breaking change. I will update this PR once 0.12 drops, or if it's merged already, I'll just make a new one that upgrades HPKE and deals with the new stuff.

@ecton
Copy link
Member

ecton commented Nov 16, 2023

I'm not primarily focused on this project at the moment, so I'm happy to wait on the 0.12 release to get this merged. There is actually a backwards compatibility test that broke due to this. I'm happy to debug this and help carry it across the finish line, but if you want to try to investigate it you can try running cargo test -p bonsaidb-local --features encryption -- self_compatibility.

Thank you again, and because I didn't say it before, thank you for the great crate!

@ecton ecton merged commit 9b8700a into khonsulabs:main Nov 27, 2023
30 of 31 checks passed
@ecton
Copy link
Member

ecton commented Nov 27, 2023

I ended up touching this project today to fix another bug I found while using it, so I've went ahead and merged this. Thank you again, and don't worry about a PR for the breaking changes in 0.12 unless you really want to send one in, I'm happy to take care of the updates.

@rozbb rozbb deleted the remove-hpke-serde branch November 27, 2023 06:19
@rozbb
Copy link
Contributor Author

rozbb commented Nov 27, 2023

Awesome! I still haven't ironed out the details for 0.12 but I'll happily send a PR when it's ready. Thanks for the eyes and the kind words!

@rozbb rozbb mentioned this pull request Jul 5, 2024
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.

2 participants