chore: optional threshold argument in user dec response validation#505
Merged
chore: optional threshold argument in user dec response validation#505
Conversation
Consolidated Tests Results 2026-04-13 - 11:22:06Test Results
Details
test-reporter: Run #3870
🎉 All tests passed!TestsView All Tests
🍂 No flaky tests in this run. Github Test Reporter by CTRF 💚 🔄 This comment has been updated |
kc1212
force-pushed
the
kc1212/chore/2966/improve-types-in-js-verification
branch
from
690e784 to
540f406
Compare
kc1212
force-pushed
the
kc1212/chore/2966/improve-types-in-js-verification
branch
from
540f406 to
f55ea42
Compare
kc1212
changed the title
kc1212
force-pushed
the
kc1212/chore/2966/improve-types-in-js-verification
branch
from
7649a4e to
6277caf
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an optional trusted threshold override for user decryption response validation (WASM path) and reorders several APIs so untrusted aggregated responses (agg_resp) are passed last, improving call-site clarity and reducing accidental misuse.
Changes:
- Add
threshold: Option<usize>to the trusted validation context and use it to drive pivot selection / validation in user decryption response validation (WASM engine). - Reorder public/client validation APIs to move
agg_respto the end (including public decryption validation and user decryption processing). - Update Rust + JS tests and WASM JS bindings to accept the new parameter, and add new unit tests covering custom-threshold validation behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| core/service/tests/js/test.js | Updates JS tests for the new WASM binding parameter ordering (threshold added). |
| core/service/src/engine/validation_wasm.rs | Adds optional threshold to trusted context and applies it in pivot/degree validation; adds unit tests. |
| core/service/src/engine/validation_non_wasm.rs | Reorders args for public decryption validation helper to put untrusted agg_resp last; updates tests. |
| core/service/src/client/user_decryption_wasm.rs | Adds threshold parameter to user decryption processing and passes it into validation context; reorders args. |
| core/service/src/client/tests/threshold/user_decryption_tests.rs | Updates call sites for reordered/extended user decryption APIs. |
| core/service/src/client/tests/threshold/public_decryption_tests.rs | Updates call sites for reordered public decryption API. |
| core/service/src/client/tests/threshold/misc_tests.rs | Updates call sites for reordered public decryption API. |
| core/service/src/client/tests/threshold/misc_tests_isolated.rs | Updates call sites for reordered public decryption API. |
| core/service/src/client/tests/centralized/user_decryption_tests.rs | Updates call sites for reordered user decryption API. |
| core/service/src/client/tests/centralized/public_decryption_tests.rs | Updates call sites for reordered public decryption API. |
| core/service/src/client/public_decryption.rs | Reorders process_decryption_resp args and updates docs and internal validation call. |
| core/service/src/client/js_api.rs | Extends WASM JS bindings to accept threshold and forwards it into the client API; doc updates. |
| core-client/src/decrypt.rs | Updates core-client call sites to match the reordered/extended client APIs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dvdplm
previously approved these changes
Apr 10, 2026
Contributor
dvdplm
left a comment
dvdplm
left a comment
There was a problem hiding this comment.
I guess it's up to us to remember to keep untrusted arguments last, but I am ok with that (until something better comes along).
I sometimes worry a bit about us overusing Option: every time we add an argument we multiply the number of code paths, making things ever so slightly trickier to reason about. There's an argument to be made about less flexibility being better in the long run. I don't think it necessarily applies here but just wanted to get my rant out. :)
The test seems solid, good comments.
dd23
reviewed
Apr 10, 2026
dd23
approved these changes
Apr 13, 2026
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
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.
Description of changes
Issue ticket number and link
Closes zama-ai/kms-internal#2966
PR Checklist
I attest that all checked items are satisfied. Any deviation is clearly justified above.
chore: ...).TODO(#issue).unwrap/expect/paniconly in tests or for invariant bugs (documented if present).devopslabel + infra notified + infra-team reviewer assigned.!and affected teams notified.Zeroize+ZeroizeOnDropimplemented.unsafe; if unavoidable: minimal, justified, documented, and test/fuzz covered.Dependency Update Questionnaire (only if deps changed or added)
Answer in the
Cargo.tomlnext to the dependency (or here if updating):More details and explanations for the checklist and dependency updates can be found in CONTRIBUTING.md