Addresses the following review comments from @Arjentix:
- "I think we can move this into `derive_from_outputs()`"
(on the position → npk map construction in main())
I moved the construction inside ExecutionState::derive_from_outputs
and stored the map as a field of ExecutionState. derive_from_outputs
now takes `private_account_keys` directly and builds the map as part
of state initialization. main() no longer owns the intermediate
structure. validate_and_sync_states reads the npk through
self.private_pda_npk_by_position.
- "Let's move this whole `is_authorized` computation into a separate
function. This became really bulky"
I extracted the caller-seeds resolution, family-binding recording,
and is_authorized computation into a free function
`resolve_authorization_and_record_bindings`. It takes the three
field borrows it needs (`&mut pda_family_binding`, `&mut
private_pda_bound_positions`, `&private_pda_npk_by_position`), same
shape as `assert_family_binding`. A method would have conflicted
with the `&mut self.post_states` borrow held by the Occupied match
arm; the free function lets rustc split-borrow the self fields.
Addresses the following review comment:
- "I think this should be a constructor `AccountId::for_private_pda`.
Consider also removing the existing `impl From<(ProgramId, Seed)> for
AccountId` for public pdas in favor of a `AccountId::for_public_pda`
to have a unified way of constructing pdas"
I replaced `impl From<(&ProgramId, &PdaSeed)> for AccountId` with
`AccountId::for_public_pda(program_id: &ProgramId, seed: &PdaSeed) ->
Self` and replaced the free function `private_pda_account_id(...)`
with `AccountId::for_private_pda(program_id: &ProgramId, seed:
&PdaSeed, npk: &NullifierPublicKey) -> Self`. Both live in an inherent
`impl AccountId` block in nssa/core/src/program.rs next to the PDA
derivation logic. Migrated all call sites across nssa/core,
nssa/src/state.rs, nssa/src/validated_state_diff.rs,
program_methods/guest/src/bin/privacy_preserving_circuit.rs,
programs/amm/core, programs/associated_token_account/core, the example
tail-call binary, and the ATA tutorial doc. Test function names that
referenced the old free function were also renamed
(private_pda_account_id_* to for_private_pda_*).
Addresses the following review comments:
- "Shouldn't we use a program that checks authorization in this test as
callee? If not, I'm not sure if we are fully testing what the test
docs describe (namely, that the callee got the input account with
is_authorized=true). Maybe add a variant of the noop that checks the
input account is authorized."
I added test_program_methods/guest/src/bin/auth_asserting_noop.rs:
same shape as noop.rs except it asserts pre.is_authorized == true for
every pre_state before echoing the post_states. Any unauthorized
pre_state panics the guest, failing the whole circuit proof. I added
Program::auth_asserting_noop() as the matching helper. In
caller_pda_seeds_authorize_private_pda_for_callee and
caller_pda_seeds_with_wrong_seed_rejects_private_pda_for_callee, I
swapped Program::noop() for Program::auth_asserting_noop() as the
callee. The positive test now proves the callee actually sees
is_authorized=true, not just that the circuit's consistency check did
not reject. The negative test doubles its evidence, both the
circuit's authorization reconciliation and the callee guest would now
reject a wrong-seed delegation.
- "This branching logic is only correct because we are not supporting
non-authorized private accounts with non-default values. Likely to be
changed in the future. I'm sure there's use cases for this. For
example the multisig program if ran completely private it would need
a private non-default and non-authorized input account."
Agreed. Supporting this needs wallet-supplied `(seed, owner)` side
input so the npk-to-account_id binding can be re-verified for an
existing private PDA without a fresh Claim::Pda or a caller
pda_seeds match. I handled this in the second PR. I added a
TODO(private-pdas-pr-2/3) marker on the `else` branch in
privacy_preserving_circuit.rs:3 => { ... } so the constraint is
visible to future maintainers, along with a comment noting the
multisig use case.
Addresses the following review comments:
- "I'd rename all mask_3 references in test names and variables to a
private pda wording. If in the future we change the mask number for
the private pda, this naming will silently get outdated."
I renamed all tests and the local variable mask3_account to
private_pda_account.
- "Let's use more descriptive names. `mask3` is not very meaningful."
I renamed all `mask3` into `private_pda`. Panic messages and .expect
strings updated to match. Doc comments that factually describe the
encoding (e.g. "mask-3 account" meaning "an account whose visibility
mask is 3") are left as-is since they are accurate and remain stable
until the mask value itself changes.
- "..._panics" to "..._fails"
Covered above. The tests assert Err(CircuitProvingError), so
execute_and_prove returns an Err, the test process itself never
panics.
- "we can return `Some((*seed, true, caller))` to avoid having to unwrap
the `caller_program_id` again in line 290"
I changed matched_caller_seed from Option<(PdaSeed, bool)> to
Option<(PdaSeed, bool, ProgramId)>, return the `caller` captured by
the enclosing and_then from each match arm, and dropped the .expect
at the consumer site. Bundled with the rename since both touch the
same branch and a single guest ELF rebuild covers them.
