Housekeeping #218

fjarri · 2025-11-03T22:59:20Z

Type of PR:

Other

Required reviews:

How many reviews does the PR author need?

2

What this does:

Update the resolver and fix clippy warnings
Bump pyo3 to 0.27
Use serde-encoded-bytes crate instead of serde_bytes and serde_with::Bytes (can revert this if you're not interested in human-readable serialization; also note that for human-readable formats everything will currently be serialized as hex strings)
Bump wasm-bindgen-derive to 0.3
Use secrecy::SecretBox

Note that I had some linker problems both for Python and JS bindings, which seem to have been fixed by the bump of Pyo3 and the explicit wasm-bindgen and js-sys dependencies in ferveo-wasm. Curious to see how CI will fare.

For reviewers:
The serialization vector tests are checking the classes exported via Python bindings (specifically, via ToBytes/FromBytes traits, which is what Python bindings use). There's a bunch of other serialization stuff that seems to be unused (e.g. ferveo_common::PublicKey::to_bytes/from_bytes) but I didn't touch it in this PR.

Copilot

Pull Request Overview

This PR migrates the codebase from a custom SecretBox implementation to using the standard secrecy crate. The change also updates several dependencies including PyO3 to version 0.27, replaces serde_bytes with serde-encoded-bytes, and updates wasm-bindgen-derive to 0.3.

Key Changes:

Replaced custom SecretBox<T> implementation with secrecy::SecretBox<T>, changing the API from as_secret()/as_mut_secret() to expose_secret()/expose_secret_mut()
Updated SecretBox to work with unsized types (SecretBox<[u8]> instead of SecretBox<Vec<u8>>)
Updated PyO3 bindings to use new API (Python::attach instead of Python::with_gil, PyString instead of PyUnicode, Bound<'_, PyModule>)
Migrated from serde_bytes to serde-encoded-bytes for hex serialization
Moved helper functions try_from_js_array and into_js_array from local implementation to wasm-bindgen-derive crate

Reviewed Changes

Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
ferveo-tdec/src/secret_box.rs	Deleted custom SecretBox implementation
ferveo-tdec/src/lib.rs	Removed secret_box module export, added secrecy import
ferveo-tdec/src/ciphertext.rs	Updated to use secrecy::SecretBox with expose_secret/expose_secret_mut methods
ferveo-tdec/src/api.rs	Changed encrypt signature to accept SecretBox<[u8]>, removed SecretBox from exports
ferveo-tdec/Cargo.toml	Added secrecy dependency, replaced serde_bytes with serde-encoded-bytes
ferveo/src/lib.rs	Moved SecretBox import to secrecy crate, updated MSG.to_vec() to MSG.into()
ferveo/src/api.rs	Updated encrypt signature and removed SecretBox export
ferveo/src/bindings_wasm.rs	Removed local try_from_js_array/into_js_array helpers, imported from wasm-bindgen-derive
ferveo/src/bindings_python.rs	Updated PyO3 API calls for 0.27 compatibility
ferveo/src/refresh.rs	Commented out broken test assertions and prefixed unused variables
ferveo/Cargo.toml	Added secrecy dependency
ferveo-wasm/tests/node.rs	Added into_js_array import from wasm-bindgen-derive
ferveo-wasm/Cargo.toml	Added wasm-bindgen and wasm-bindgen-derive dependencies
ferveo-python/src/lib.rs	Updated to use Bound<'_, PyModule> parameter
ferveo-python/ferveo/init.pyi	Changed init to new in type stubs
ferveo-python/Cargo.toml	Removed extension-module feature configuration
ferveo-common/src/serialization.rs	Migrated from serde_with::Bytes to serde-encoded-bytes SliceLike
ferveo-common/Cargo.toml	Added serde-encoded-bytes dependency
Cargo.toml	Updated workspace dependencies (pyo3 to 0.27, wasm-bindgen-derive to 0.3, added secrecy and serde-encoded-bytes)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-04T21:43:20Z

ferveo/src/bindings_wasm.rs

 ) -> JsResult<Vec<(api::Validator, api::Transcript)>> {
-    let messages = try_from_js_array::<ValidatorMessage>(messages)?;
+    let messages =
+        try_from_js_array::<ValidatorMessage>(messages).map_err(map_js_err)?;


The try_from_js_array function is now imported from wasm-bindgen-derive and its error handling has changed. The function now returns a Result that needs to be mapped with map_err(map_js_err). However, checking the usage pattern across the file, this error mapping is inconsistent with the removal of the old local implementation. The old implementation already had fmt::Display constraint that worked with map_js_err, but the new version from the derive crate may have a different error type. Verify that the error type from wasm-bindgen-derive::try_from_js_array implements fmt::Display and is compatible with map_js_err.

Suggested change

try_from_js_array::<ValidatorMessage>(messages).map_err(map_js_err)?;

try_from_js_array::<ValidatorMessage>(messages).map_err(|e| map_js_err(e.to_string()))?;

Strange take from the copilot. map_js_err has a Display bound on its argument, if it compiles, it's going to work.

Copilot · 2025-11-04T21:43:20Z

ferveo-tdec/src/ciphertext.rs

 ) -> Result<ChaCha20Poly1305> {
-    let mut prf_key = SecretBox::new(Vec::new());
+    let mut prf_key = SecretBox::new(
+        vec![0u8; shared_secret.0.compressed_size()].into_boxed_slice(),


Creating a zero-initialized vector and then converting it to a boxed slice is inefficient. Consider using vec![0u8; shared_secret.0.compressed_size()].into_boxed_slice() or better yet, directly creating a boxed slice with Box::new([0u8; N]) if the size is known at compile time, or using Box::<[u8]>::new_zeroed_slice(n).assume_init() for runtime-known sizes.

Suggested change

vec![0u8; shared_secret.0.compressed_size()].into_boxed_slice(),

// SAFETY: new_zeroed_slice returns a Box<[u8]> where all bytes are zeroed.

unsafe { Box::<[u8]>::new_zeroed_slice(shared_secret.0.compressed_size()).assume_init() },

Not sure using unsafe is better than creating a vector. I don't think there's a way to get a dynamically allocated box with a given size directly, is there?

Copilot · 2025-11-04T21:43:21Z

ferveo/src/refresh.rs

        //     )
        //     .unwrap();
-        assert_ne!(original_private_key_share, original_private_key_share);
+        // assert_ne!(original_private_key_share, original_private_key_share);


This commented-out assertion assert_ne!(original_private_key_share, original_private_key_share) would always fail since it compares a value to itself. This indicates the test logic is incomplete or incorrect. Either remove this assertion entirely or fix it to compare the correct values (e.g., comparing the original share with a refreshed share).

Suggested change

// assert_ne!(original_private_key_share, original_private_key_share);

// assert_ne!(incorrect_private_key_share, original_private_key_share);

^ @cygnusv ... huh?

This is a skipped test, so this isn't executed anyway. I commented the assertion because it was triggering a clippy warning (naturally). Not sure what's going on there.

Ah ok. I missed that it was skipped.

Copilot · 2025-11-04T21:43:21Z

ferveo/src/refresh.rs

        //     )
        //     .unwrap();
-        assert_ne!(shared_private_key, shared_private_key);
+        // assert_ne!(shared_private_key, shared_private_key);


This commented-out assertion assert_ne!(shared_private_key, shared_private_key) would always fail since it compares a value to itself. This indicates the test logic is incomplete or incorrect. Either remove this assertion entirely or fix it to compare the correct values (e.g., comparing the original key with a refreshed key).

derekpierre · 2025-11-04T22:09:34Z

ferveo-tdec/src/ciphertext.rs


    // V
-    #[serde(with = "serde_bytes")]
+    #[serde(with = "SliceLike::<Hex>")]


Is this specifically serializing as hex instead of bytes? Backwards compatibility?

It's serializing as hex in human-readable formats, and as bytes in binary formats. Same as in umbral. The binary format ABI won't change, unless serde_bytes was saving constant-sized byte arrays as serialize_tuple() in serde, which leads to inefficient serialization. I suppose I need to add vector tests here as well.

Yeah. Some vector tests would be good for reinforcement.

Added a big serialization vector test in ferveo, before the serde-encoded-bytes commit.

derekpierre · 2025-11-04T22:12:23Z

ferveo/src/refresh.rs

        //     )
        //     .unwrap();
-        assert_ne!(original_private_key_share, original_private_key_share);
+        // assert_ne!(original_private_key_share, original_private_key_share);


^ @cygnusv ... huh?

fjarri · 2025-11-06T18:56:30Z

This is RFR

fjarri force-pushed the housekeeping branch from d543482 to c003893 Compare November 3, 2025 23:13

fjarri mentioned this pull request Nov 4, 2025

Housekeeping nucypher/nucypher-core#122

Open

fjarri force-pushed the housekeeping branch from c003893 to 26aac7a Compare November 4, 2025 07:47

derekpierre requested review from KPrasch, Copilot, cygnusv and derekpierre November 4, 2025 21:41

Copilot AI reviewed Nov 4, 2025

View reviewed changes

derekpierre reviewed Nov 4, 2025

View reviewed changes

fjarri force-pushed the housekeeping branch from 484f881 to 3ca1051 Compare November 6, 2025 00:30

fjarri added 2 commits November 6, 2025 00:24

Add the explicit resolver, bump pyo3, fix clippy and build issues

a2477fc

Bump wasm-bindgen-derive to 0.3

44de65e

fjarri force-pushed the housekeeping branch from 3ca1051 to b5e4498 Compare November 6, 2025 08:24

fjarri added 4 commits November 6, 2025 12:37

Add vector tests

b44d7f9

Use serde-encoded-bytes

815fd8d

Use secrecy

b71c2f6

Fix Python stubs

d77120f

fjarri force-pushed the housekeeping branch from b5e4498 to d77120f Compare November 6, 2025 20:39

fjarri mentioned this pull request Nov 6, 2025

Api fixes #220

Open

fjarri mentioned this pull request Dec 6, 2025

Remove an explicit conversion to bytes of the result of ThresholdMessageKit.decrypt_with_shared_secret() fjarri/nucypher-async#39

Open

	try_from_js_array::<ValidatorMessage>(messages).map_err(map_js_err)?;
	try_from_js_array::<ValidatorMessage>(messages).map_err(\|e\| map_js_err(e.to_string()))?;

	vec![0u8; shared_secret.0.compressed_size()].into_boxed_slice(),
	// SAFETY: new_zeroed_slice returns a Box<[u8]> where all bytes are zeroed.
	unsafe { Box::<[u8]>::new_zeroed_slice(shared_secret.0.compressed_size()).assume_init() },

	// assert_ne!(original_private_key_share, original_private_key_share);
	// assert_ne!(incorrect_private_key_share, original_private_key_share);

Housekeeping #218

Are you sure you want to change the base?

Housekeeping #218

Uh oh!

Conversation

fjarri commented Nov 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Key Changes:

Reviewed Changes

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 4, 2025

Choose a reason for hiding this comment

Uh oh!

derekpierre Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjarri commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fjarri commented Nov 3, 2025 •

edited

Loading

derekpierre Nov 4, 2025 •

edited

Loading