-
Notifications
You must be signed in to change notification settings - Fork 20
Housekeeping #135
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
base: master
Are you sure you want to change the base?
Housekeeping #135
Conversation
|
Probably will need to bump MSRV, will know when someone enables CI for me :) |
e6ba497 to
fe0f82b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #135 +/- ##
==========================================
- Coverage 58.17% 56.78% -1.40%
==========================================
Files 17 16 -1
Lines 3118 2520 -598
==========================================
- Hits 1814 1431 -383
+ Misses 1304 1089 -215 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
fb37eb7 to
50baf5f
Compare
| pub(crate) struct KeyFragID(GenericArray<u8, KeyFragIDSize>); | ||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| pub(crate) struct KeyFragID( | ||
| #[cfg_attr(feature = "serde", serde(with = "GenericArray014::<Hex>"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To confirm, the actual serialization doesn't change, correct? i.e. the serialized bytes remain the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be the same - it's still serialized as a byte slice (serde got a long-standing problem with constant-sized arrays). The container type (GenericArray014) only serves to define the specific AsRef and TryFrom impls that will be used for (de)serialization, because static arrays, slices, and GenericArray types have different ones.
I can test it out though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let’s double-check. I'd like to ensure that there isn’t a backward compatibility issue where data serialized using the previous code can’t be properly deserialized after this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so in the rebased PR I added a "vector tests" commit just before the switch to serde-encoded-bytes, checking against hardcoded serialization results, and the tests still pass after the switch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR migrates from a custom SecretBox implementation to the secrecy crate and refactors serialization using serde-encoded-bytes. Key changes include:
- Replaced custom
SecretBoxandserde_bytesmodules withsecrecycrate (v0.10) andserde-encoded-bytes(v0.2) - Updated API calls from
as_secret()/as_mut_secret()toexpose_secret()/expose_secret_mut() - Changed
SecretBox::new()toSecretBox::init_with()for initialization - Updated PyO3 bindings from v0.18 to v0.27 with corresponding API changes (
PyModule→Bound<'_, PyModule>,PyObject→Py<PyAny>,with_gil→attach) - Updated wasm-bindgen-derive from v0.2 to v0.3
- Bumped MSRV from 1.70.0 to 1.81.0
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| umbral-pre/src/serde_test.rs | Added new test helper module extracting serialization roundtrip testing |
| umbral-pre/src/serde_bytes.rs | Deleted custom serde serialization utilities |
| umbral-pre/src/secret_box.rs | Deleted custom SecretBox implementation |
| umbral-pre/src/lib.rs | Updated module declarations and removed SecretBox export |
| umbral-pre/src/keys.rs | Migrated to secrecy crate API and serde-encoded-bytes |
| umbral-pre/src/key_frag.rs | Updated SecretBox usage and serialization |
| umbral-pre/src/hashing.rs | Updated SecretBox API calls |
| umbral-pre/src/evidence.rs | Updated serde attribute for serialization |
| umbral-pre/src/dem.rs | Updated SecretBox initialization patterns |
| umbral-pre/src/curve.rs | Migrated serialization to serde-encoded-bytes |
| umbral-pre/src/capsule_frag.rs | Updated SecretBox usage |
| umbral-pre/src/capsule.rs | Updated SecretBox initialization and exposure |
| umbral-pre/src/pre.rs | Updated SecretBox API calls |
| umbral-pre/src/bindings_wasm.rs | Moved helper functions to wasm-bindgen-derive crate |
| umbral-pre/src/bindings_python.rs | Updated PyO3 API for v0.27 compatibility |
| umbral-pre/src/bench.rs | Updated imports |
| umbral-pre/Cargo.toml | Added new dependencies and updated versions |
| umbral-pre-python files | Updated PyO3 v0.27 compatibility |
| Cargo.toml | Set resolver to version 2 |
| .github/workflows/umbral-pre.yml | Updated MSRV to 1.81.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # These packages are among the dependencies of the packages above. | ||
| # Their versions should be updated when the main packages above are updated. | ||
| generic-array = { version = "0.14.6", features = ["zeroize"] } | ||
| generic-array = { version = "=0.14.7", features = ["zeroize"] } |
Copilot
AI
Nov 4, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using an exact version pin (=0.14.7) for generic-array is very restrictive and may cause dependency conflicts in downstream crates. Consider using a more flexible version constraint like '0.14.7' or '^0.14.7' unless there's a specific reason for the exact pin.
| generic-array = { version = "=0.14.7", features = ["zeroize"] } | |
| generic-array = { version = "0.14.7", features = ["zeroize"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately the author of generic-array screwed everyone by adding deprecation notices to every single API item in 0.14.8 (in an effort to make people switch to 1.x I guess, see fizyk20/generic-array#158). But the RustCrypto stack libraries we depend on still use 0.14, so we can't upgrade.
a8bb266 to
ca99e2a
Compare
Type of PR:
Required reviews:
What this does:
Housekeeping tasks:
pyo3to 0.27serde-encoded-bytescrate instead ofserde_bytesmodulesecrecycrate instead ofsecret_boxmodulewasm-bindgen-deriveto 0.3zeroizeto 1.8