refactor(PR 12): add SecretFr, WasmSecretFr, FFI_SecretFr to zerokit, standardize type naming in Wasm and FFI#423
Merged
Conversation
…follow convention
…cretFr opaque type, standardize wasm type naming
…etFr opaque type, standardize FFI type naming
Benchmark for 725d718Click to view benchmark
|
Benchmark for 7871c1cClick to view benchmark
|
Benchmark for 725d718Click to view benchmark
|
Benchmark for 7871c1cClick to view benchmark
|
vinhtc27
changed the title
vinhtc27
changed the title
vinhtc27
changed the title
sydhds
requested changes
Jul 6, 2026
| @@ -481,7 +481,7 @@ mod test { | |||
| let leaf_index = 3; | |||
|
|
|||
| let identity_secret_seed = hash_to_field_le(b"test-merkle-proof"); | |||
Contributor
There was a problem hiding this comment.
Why not using let mut then avoiding the clone here?
| /// | ||
| /// The internal copy of the secret is zeroized after hashing. | ||
| pub(crate) fn poseidon_hash_id_secret(secret: &SecretFr) -> Fr { | ||
| let mut to_hash = [**secret]; |
Contributor
There was a problem hiding this comment.
deref here would be more readable wdyt?
Contributor
Author
There was a problem hiding this comment.
It still need * and then .deref so i guess it kinda the same, and also have to import some trait so i prefer this.
|
|
||
| let expected_identity_secret = poseidon_hash_pair(trapdoor, nullifier); | ||
| let mut to_hash = [identity_secret]; | ||
| let expected_identity_secret = poseidon_hash_pair(*trapdoor, *nullifier); |
Contributor
There was a problem hiding this comment.
Why not use the hash pair for Secret Fr here ?
| #[wasm_bindgen(js_name = newSingle)] | ||
| pub fn new_single( | ||
| identity_secret: &WasmFr, | ||
| identity_secret: &WasmSecretFr, |
Contributor
There was a problem hiding this comment.
I'm seeing this only now but is there a reason we use reference here everywhere? Because, for example, it requires to clone the secret here so almost no reason no?
Contributor
Author
There was a problem hiding this comment.
Yes it has a good reason to do so.
WASM take & for all non-primitive types because with wasm-bindgen, a by-value param moves ownership, the JS object gets its internal pointer nulled after the call. JS has no move semantics, so callers don't expect this:
const keys = WasmIdentityKeys.generate();
const secret = keys.getSecret();
const w1 = WasmRLNWitnessInput.newSingle(secret, ...); // if by-value: secret is consumed here
const w2 = WasmRLNWitnessInput.newSingle(secret, ...); // throws "null pointer passed to rust"With & the object stays alive and we clone once inside instead. Otherwise we need to provide clone() fn.
The same idea apply for FFI too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes: