feat: strong-type PrivacyPreservingCircuitInput with per-account enum#462
feat: strong-type PrivacyPreservingCircuitInput with per-account enum#462moudyellaz wants to merge 5 commits intomainfrom
Conversation
moudyellaz
force-pushed
the
moudy/feat-strong-type-circuit-input
branch
3 times, most recently
from
1bd9455 to
f734965
Compare
Pravdyvy
left a comment
Pravdyvy
left a comment
There was a problem hiding this comment.
Thank you for PR. Left one suggestion.
| /// Per-account input to the privacy-preserving circuit. Each variant carries exactly the fields | ||
| /// the guest needs for that account's code path. | ||
| #[derive(Serialize, Deserialize, Clone)] | ||
| pub enum PrivacyPreservingCircuitInputAccount { |
There was a problem hiding this comment.
I think this naming is ambiguous on the size of variable. We have now 2 accounts types, one holds keys, other holds data and balance. I propose to rename this enum into something like PrivacyPreservingCircuitInputAccountIdentity, for example PrivacyPreservingCircuitInputAccountIdentity and name variables of this enum account_identity.
There was a problem hiding this comment.
Agree. Also I think we can drop the PrivacyPreservingCircuit prefix as it already is defined inside circuit_io
There was a problem hiding this comment.
Done, renamed the enum to InputAccountIdentity, variant-of-this-enum variables to account_identity, and the struct field to account_identities.
There was a problem hiding this comment.
Done, dropped the PrivacyPreservingCircuit prefix. The type is now InputAccountIdentity, re-exported from nssa_core::lib.rs.
schouhy
left a comment
schouhy
left a comment
There was a problem hiding this comment.
Thanks for the PR. This is a major improvement for readability of the code. Left minor comments
| /// Public account. The guest reads pre/post state from `program_outputs` and emits no | ||
| /// commitment, ciphertext, or nullifier. | ||
| Public, | ||
| /// Init of an authorized standalone private account (mask 1, no membership proof). The |
There was a problem hiding this comment.
I'd remove all mentions to mask as that's now legacy and would be confusing for someone without context of previous implementations
| // The four `circuit_fails_if_insufficient_*_are_provided` tests have been removed in this | ||
| // refactor: each variant of `PrivacyPreservingCircuitInputAccount` carries exactly the fields | ||
| // the corresponding circuit code path needs, so the parallel-vec length mismatches those | ||
| // tests exercised are now compile-time impossibilities. |
There was a problem hiding this comment.
agreed with removing the tests. But let's not add this comment here. Let's put it in the PR description instead
There was a problem hiding this comment.
Done, removed the in-code comments and added a Removed tests section to the PR description.
- Rename PrivacyPreservingCircuitInputAccount to InputAccountIdentity (drop the PrivacyPreservingCircuit prefix; add Identity suffix) - Rename PrivacyPreservingCircuitInput.accounts to account_identities - Rename AccountManager.accounts() to account_identities() and loop variables to account_identity - Drop legacy mask-1/2/3 references from variant doc comments and guest comments - Remove the explanatory comments about deleted parallel-vec tests; moved to the PR description - Rebake privacy_preserving_circuit and test program artifacts
3 participants
🎯 Purpose
Refactor
PrivacyPreservingCircuitInputfrom four parallel vectors (visibility_mask,private_account_keys,private_account_nsks,private_account_membership_proofs) into a singleVec<InputAccountIdentity>enum, eliminating implicit lock-step iteration and runtime length-coupling assertions in the privacy circuit guest. Closes #455.⚙️ Approach
InputAccountIdentityenum innssa/core/src/circuit_io.rswith six variants:Public,PrivateAuthorizedInit,PrivateAuthorizedUpdate,PrivateUnauthorized,PrivatePdaInit,PrivatePdaUpdate.PrivacyPreservingCircuitInputwith a singleaccount_identities: Vec<...>field aligned 1:1 with the program's pre_states.compute_circuit_outputandderive_from_outputsin the privacy circuit guest to consumeaccount_identitiesdirectly, dropping the four lock-step iterators and trailing length asserts.execute_and_provehost signature to takeaccount_identities.nssa/src/state.rs,nssa/src/privacy_preserving_transaction/circuit.rs, sequencer test, and integration test.AccountManager(wallet/src/privacy_preserving_tx.rs) to exposeaccount_identities()directly, eliminating the parallel-vec accessors and the wallet-side iterator-flatten that would have replicated the same anti-pattern at the wallet/circuit boundary.privacy_preserving_circuit.bin.🧪 How to Test
RISC0_DEV_MODE=1 cargo test --releaseonnssa-coreandnssa.🔗 Dependencies
None.
🔜 Future Work
The "authorized init of a private PDA" sub-case (an authorized PDA pre-state that is still default) is structurally allowed by the guest's private-PDA paths today but is not produced by any wallet code. If a future PR enables it via
(seed, owner)side input for top-level reuse, the variant set will need to be revisited.🗑️ Removed tests
Seven tests have been removed because each one exercised a parallel-vec length mismatch that the new
Vec<InputAccountIdentity>makes structurally impossible:circuit_fails_if_insufficient_nonces_are_providedcircuit_fails_if_insufficient_keys_are_providedcircuit_fails_if_insufficient_commitment_proofs_are_providedcircuit_fails_if_insufficient_auth_keys_are_providedcircuit_should_fail_with_too_many_noncescircuit_should_fail_with_too_many_private_account_keyscircuit_should_fail_with_too_many_private_account_auth_keysEach variant of
InputAccountIdentitycarries exactly the fields the corresponding circuit code path needs, so over-supply or under-supply of any per-account field is a compile-time error rather than a runtime panic.📋 PR Completion Checklist