test(privacy): fix AMM clock ownership bug, confirm 5 chained-call privacy tests, add 5 more

Root cause: advance_clock() left the clock account DEFAULT_PROGRAM_ID-owned, which
spel-framework's guest dispatcher (upstream in logos-co/spel) silently drops from a
program's output as an unclaimed, non-default, default-owned account. That desynced
the privacy circuit's account count from the caller-supplied identities, blocking
every AMM chained-call privacy test with "Invalid account_identities length".

Fix: own the fixture's clock account with a placeholder non-default ProgramId.
Rewrote the 5 previously not-expressible Swap/AddLiquidity/RemoveLiquidity privacy
tests to assert real success (transaction applied, resulting state and private
commitments verified) now that they're unblocked.

Added 5 more tests: Swap to a fresh PrivateUnauthorized/PrivateAuthorizedInit
destination (both confirmed not-expressible, for two distinct reasons — guest ABI
signer requirement vs. destination-must-already-exist), and NewDefinition with a
private PrivateAuthorizedInit/PrivateUnauthorized initial LP holder (expressible for
the former, not for the latter).

Updated docs/findings.md and docs/privacy-test-matrix.md accordingly.
This commit is contained in:
Marvin Jones
2026-07-15 14:06:20 -04:00
parent 6097f13f64
commit 8f2d937a9d
3 changed files with 896 additions and 81 deletions
+15 -9
View File
@@ -78,16 +78,20 @@ In this task, we extend testing for LEZ programs to cover privacy features:
| Function tested | Test name | Category | Description of objective | Result |
|---|---|---|---|---|
| SwapExactInput | `amm_swap_a_to_b_private_user_holding_is_not_expressible` | REGULAR, CHAIN | Private `user_holding_a` deposit leg — confirms the circuit-level account-count bug also fires with a real private account (8 vs 7 accounts), not just the all-public control case | ❌ (confirmed not-expressible — circuit bug) |
| SwapExactOutput | `amm_swap_exact_output_private_user_holding_is_not_expressible` | REGULAR, CHAIN | Same confirmation for `SwapExactOutput` — identical account/chained-call shape to `SwapExactInput` (8 vs 7 accounts) | ❌ (confirmed not-expressible — circuit bug) |
| AddLiquidity | `amm_add_liquidity_private_lp_holding_is_not_expressible` | REGULAR, CHAIN | Private LP-output holding (`user_holding_lp`) — same circuit bug (10 vs 9 accounts) | ❌ (confirmed not-expressible — circuit bug) |
| AddLiquidity | `amm_add_liquidity_private_user_holdings_is_not_expressible` | REGULAR, CHAIN | Private deposit legs (`user_holding_a` + `user_holding_b`) — same circuit bug (10 vs 9 accounts) | ❌ (confirmed not-expressible — circuit bug) |
| RemoveLiquidity | `amm_remove_liquidity_private_lp_holding_is_not_expressible` | REGULAR, CHAIN | Private LP holding (the account that signs/burns to remove liquidity) — same circuit bug (10 vs 9 accounts) | ❌ (confirmed not-expressible — circuit bug) |
| RemoveLiquidity | `amm_remove_liquidity_private_new_user_holdings_is_not_expressible` | EXIST, CHAIN | Brand-new `PrivateUnauthorized` token A/B destinations — rejected by a separate, unrelated program-level precondition (destination must already exist) before the circuit bug is even reached | ❌ (confirmed not-expressible — different reason) |
| SwapExactInput | `amm_swap_a_to_b_private_user_holding` | REGULAR, CHAIN | Private `user_holding_a` deposit leg, through the Token + TWAP-oracle chained calls | ✅ |
| SwapExactOutput | `amm_swap_exact_output_private_user_holding` | REGULAR, CHAIN | Same coverage for `SwapExactOutput` | ✅ |
| AddLiquidity | `amm_add_liquidity_private_lp_holding` | REGULAR, CHAIN | Private LP-output holding (`user_holding_lp`) receives newly-minted LP on top of an existing private balance | ✅ |
| AddLiquidity | `amm_add_liquidity_private_user_holdings` | REGULAR, CHAIN | Private deposit legs (`user_holding_a` + `user_holding_b`), public LP recipient | ✅ |
| RemoveLiquidity | `amm_remove_liquidity_private_lp_holding` | REGULAR, CHAIN | Private LP holding (the account that signs/burns to remove liquidity) | ✅ |
| RemoveLiquidity | `amm_remove_liquidity_private_new_user_holdings_is_not_expressible` | EXIST, CHAIN | Brand-new `PrivateUnauthorized` token A/B destinations — rejected by a separate, unrelated program-level precondition (destination must already exist) | ❌ (confirmed not-expressible — different reason) |
| SwapExactInput | `amm_swap_a_to_b_private_unauthorized_destination_is_not_expressible` | EXIST, CHAIN | Swap paying out to a brand-new `PrivateUnauthorized` destination (`npk` only, no `nsk`) | ❌ (confirmed not-expressible — guest ABI requires both swap legs to be signers, which `PrivateUnauthorized` can never satisfy by construction) |
| SwapExactInput | `amm_swap_a_to_b_private_authorized_init_destination_is_not_expressible` | REGULAR, CHAIN | Swap paying out to a brand-new `PrivateAuthorizedInit` destination (owner self-initializes with its own `nsk`) | ❌ (confirmed not-expressible — same "destination must already exist" precondition as `RemoveLiquidity`) |
| NewDefinition | `amm_new_definition_private_initial_lp_holder` | REGULAR | Pool creation with a private `PrivateAuthorizedInit` initial LP holder | ✅ |
| NewDefinition | `amm_new_definition_private_unauthorized_lp_holder_is_not_expressible` | EXIST, REGULAR | Pool creation with a `PrivateUnauthorized` initial LP holder (`npk` only, no `nsk`) | ❌ (confirmed not-expressible — guest ABI requires `user_holding_lp` to be a signer, which `PrivateUnauthorized` can never satisfy; same shape as the `Swap` `PrivateUnauthorized` finding above) |
### Remarks
- `RemoveLiquidity` and `Swap`s may have issues with `PrivateUnauthorized` and `PrivateAuthorizedInit` that match issues detected in Stablecoin; e.g., explicitly requires `is_authorized = true` and non default accounts.
- `clock` account issue: clock is silent dropped during privacy executions.
- `Swap` and `Remove` rejects any uninitialized destination account; this is a AMM design choice, and not Token program requirement.
- AMM tests were initially blocked by a bug.
## ATA program
@@ -156,4 +160,6 @@ ATA program offers limited usage with private accounts. Private accounts can be
Privacy coverage for LEZ program tests is greatly improved from the added tests. Though, there are a few noticable gaps:
- `PrivateUnauthorized` accounts can be blocked by programs with a check `is_authorized = true`. However, this issue can be avoided by defining `is_authorized = true` for account initialization with `PrivateUnauthorized` (e.g., no knowledge of `npk`). Account initialization cannot be used to maliciously alter a pre-existing account, and thus `is_authorized = true` would not offer any malicious path forward for the third-party initializing the account.
- Privacy transactions have issues with chain calls in which multiple calls affect the same private account. This issue can be mitigated by adopting account diff paradigm instead of the current "account state replacement" that we currently use.
- AMM tests are blocked by issues with the clock account; bug in `spel-framework`.
Additional observation:
AMM's chained-call privacy tests were blocked by the clock account being `DEFAULT_PROGRAM_ID`-owned in the test fixture, which trips a `spel-framework` dispatcher bug (upstream in `logos-co/spel`, confirmed present through v0.6.0) that silently drops any default-owned, non-default, unclaimed account from a program's output. Fixed by giving the fixture's clock account a non-default owner; see the AMM section. The dispatcher bug itself remains open upstream.
+66 -18
View File
@@ -542,13 +542,58 @@ arbitrary timestamp as a private witness and no check anywhere would catch it. W
to the LEZ/SPEL maintainers independent of whether/when the AMM test-writing blocker itself gets
prioritized.
**Upstream provenance checked (2026-07-15)**: cloned both `logos-co/spel` (upstream) and
`0x-r4bbit/spel` (the pinned fork) to check whether the filter was fork-introduced. It isn't.
`git log -S"is_default_owner"` on upstream `logos-co/spel` finds it added in PR #126, squash-merged
to upstream `main` as commit `1f51875` ("`SpelOutput::execute()` with auto-claim support"), still
present at upstream's current HEAD (`0cb7e09`, v0.6.0). Walking the fork's history back to its
merge point from `logos-co/release/v0.5.0` (commit `73fc462`) shows the filter already present
there too, before any of the fork's own commits. **This is an upstream `logos-co/spel` bug**,
inherited unchanged by the pinned fork — report it against the former, not the latter.
### ✅ Fixed for these tests (2026-07-15) — test-fixture clock ownership, not a circuit workaround
The immediate blocker for all five `CHAIN`-dimension AMM privacy tests below was that
`advance_clock` (`programs/integration_tests/tests/amm.rs`) constructed the clock account with
`..Account::default()`, leaving `program_owner == DEFAULT_PROGRAM_ID` and tripping the
`spel-framework` filter above on every AMM/TWAP call that echoes it back. The *real* production
clock account is owned by a dedicated clock program (`lez/system_accounts/src/lib.rs`'s
`clock_account()`: `program_owner: programs::clock().id()`), not `DEFAULT_PROGRAM_ID` — but that
constructor lives behind the gated `system-programs`/`artifacts` feature this test crate can't
reach, so `advance_clock` fabricates the account directly and never set an owner.
Fix applied: `advance_clock` now sets `program_owner` to a placeholder non-default `ProgramId`
(`[42_u32; 8]`, a stand-in — not the real production clock program ID, which isn't reachable from
this crate). Nothing in AMM/TWAP-oracle logic checks the clock's `program_owner` value (only its
`account_id` against `CLOCK_01_PROGRAM_ACCOUNT_ID`), and `validate_execution`'s other rules
(no-ownership-change, no-unauthorized-data/balance-change) are satisfied trivially since clock is
echoed unchanged — so this is a safe, minimal, test-only fix. All five previously-blocked tests
were rewritten from `_is_not_expressible`/`expect_err` assertions to real success assertions
(rebuilding the `PrivacyPreservingTransaction`, applying it via
`transition_from_privacy_preserving_transaction`, and checking resulting public state +
commitments for the private accounts) and now pass. Full `amm.rs` suite: 40/40 passing.
The `spel-framework` dispatcher filter itself is untouched and remains an open upstream bug
(confirmed to originate in `logos-co/spel`, not the pinned fork — see above) — it would still
silently drop any other `DEFAULT_PROGRAM_ID`-owned, non-default, unclaimed account threaded
through a chained call. Worth reporting upstream regardless of this fixture-level fix.
### Existing
6 private tests out of 33 pre-existing public + 6 = 39. No test can yet demonstrate an
actually-working AMM privacy path — five exist purely to confirm the circuit bug also blocks
real private accounts (not just the all-public control case), and one
(`amm_remove_liquidity_private_new_user_holdings_is_not_expressible`) found a second, distinct,
earlier blocker specific to `RemoveLiquidity`.
10 private tests out of 34 pre-existing public + 10 = 44. Six demonstrate an actually-working AMM
privacy path end-to-end (private account touched, transaction applied, resulting public state and
private commitments verified): the five chained-call tests unblocked by the clock test-fixture fix
above, plus `amm_new_definition_private_initial_lp_holder` (pool creation with a fresh
`PrivateAuthorizedInit` LP holder — `new_definition.rs` explicitly permits this, unlike
`swap`/`remove`). Four are confirmed not-expressible, splitting into two distinct root causes:
`amm_remove_liquidity_private_new_user_holdings_is_not_expressible` and
`amm_swap_a_to_b_private_authorized_init_destination_is_not_expressible` both hit the same
"destination must already exist and be Token-Program-owned" precondition;
`amm_swap_a_to_b_private_unauthorized_destination_is_not_expressible` and
`amm_new_definition_private_unauthorized_lp_holder_is_not_expressible` both hit an earlier, more
fundamental guest-ABI signer requirement that a `PrivateUnauthorized` identity can never satisfy —
any account declared a required signer in the guest ABI structurally excludes `PrivateUnauthorized`
recipients, regardless of what the program's own internal precondition would otherwise allow.
**Second finding, unrelated to the circuit bug (2026-07-13)**: `remove_liquidity` requires
`user_holding_a`/`user_holding_b` to already exist and already be owned by the configured Token
@@ -568,25 +613,28 @@ program-level precondition that predates privacy entirely, not a circuit artifac
| Instruction | Dimension | Test | Priority | Depends on | Status |
|---|---|---|---|---|---|
| SwapExactInput | `CHAIN` | `amm_swap_a_to_b_private_user_holding_is_not_expressible` | P1 | Token, TWAP oracle (public leg) | **Confirmed not-expressible** — private `user_holding_a`, fails identically to the all-public control (8 vs 7 accounts) |
| SwapExactOutput | `CHAIN` | `amm_swap_exact_output_private_user_holding_is_not_expressible` | P1 | Token, TWAP oracle (public leg) | **Confirmed not-expressible**identical 8-account/chained-call shape to `SwapExactInput`, fails identically (8 vs 7 accounts) |
| AddLiquidity | `CHAIN` | `amm_add_liquidity_private_user_holdings_is_not_expressible` — private deposit legs (`user_holding_a`/`user_holding_b`) | P1 | Token, TWAP oracle (public leg) | **Confirmed not-expressible**fails identically (10 vs 9 accounts) |
| AddLiquidity | BASE | `amm_add_liquidity_private_lp_holding_is_not_expressible` — private LP output holding | P1 | Token | **Confirmed not-expressible** — private `user_holding_lp`, fails identically (10 vs 9 accounts) |
| RemoveLiquidity | `CHAIN` | `amm_remove_liquidity_private_lp_holding_is_not_expressible` | P1 | Token, TWAP oracle (public leg) | **Confirmed not-expressible** — private `user_holding_lp`, fails identically (10 vs 9 accounts) |
| RemoveLiquidity | `EXIST` (negative) | `amm_remove_liquidity_private_new_user_holdings_is_not_expressible` — brand-new `PrivateUnauthorized` token A/B destinations | P1 | Token | **Confirmed not-expressible for a different reason** — AMM's own precondition requires the destination to already be owned by the Token Program; fails before the circuit bug is even reached |
| Swap / AddLiquidity | `EXIST` | `amm_swap_into_existing_private_holding` | P2 | Token | **Blocked** — see above |
| NewDefinition | BASE | `amm_new_definition_private_initial_lp_holder` | P2 | Token | **Blocked** — see above (also issues chained calls reusing `pool`-derived accounts; check on resolution) |
| Swap / AddLiquidity (vault) | `PDA` | `amm_swap_with_private_vault_pda` — predicted **not-expressible** per the ATA `PDA` finding (same `for_public_pda`-only root cause, confirmed in `amm_core`); write as a quick confirmation citing that finding, not a fresh investigation | P2 | Token | Not started (also behind the blocker above) |
| AddLiquidity / RemoveLiquidity | `GROUP` | `amm_group_owned_lp_holding` | P3 | Token, `key_protocol` | **Blocked** — see above |
| SwapExactInput | `CHAIN` | `amm_swap_a_to_b_private_user_holding` | P1 | Token, TWAP oracle (public leg) | **Expressible** — private `user_holding_a`, unblocked by the clock test-fixture fix |
| SwapExactOutput | `CHAIN` | `amm_swap_exact_output_private_user_holding` | P1 | Token, TWAP oracle (public leg) | **Expressible**same fix |
| AddLiquidity | `CHAIN` | `amm_add_liquidity_private_user_holdings` — private deposit legs (`user_holding_a`/`user_holding_b`) | P1 | Token, TWAP oracle (public leg) | **Expressible**same fix |
| AddLiquidity | BASE | `amm_add_liquidity_private_lp_holding` — private LP output holding | P1 | Token | **Expressible** — private `user_holding_lp`, same fix |
| RemoveLiquidity | `CHAIN` | `amm_remove_liquidity_private_lp_holding` | P1 | Token, TWAP oracle (public leg) | **Expressible** — private `user_holding_lp`, same fix |
| RemoveLiquidity | `EXIST` (negative) | `amm_remove_liquidity_private_new_user_holdings_is_not_expressible` — brand-new `PrivateUnauthorized` token A/B destinations | P1 | Token | **Confirmed not-expressible for a different reason** — AMM's own precondition requires the destination to already be owned by the Token Program; unrelated to the clock issue |
| Swap | `EXIST` (negative) | `amm_swap_a_to_b_private_unauthorized_destination_is_not_expressible` — brand-new `PrivateUnauthorized` recipient (`npk` only) | P1 | Token | **Confirmed not-expressible** — the guest ABI (`#[account(mut, signer)]` on both `user_holding_a`/`user_holding_b` in `methods/guest/src/bin/amm.rs`) requires every swap participant to be a signer; `PrivateUnauthorized` is authorized by nobody by construction, so it can never satisfy this, independent of the destination-must-exist issue below |
| Swap | `EXIST` (negative) | `amm_swap_a_to_b_private_authorized_init_destination_is_not_expressible` — brand-new `PrivateAuthorizedInit` recipient (self-initializes with its own `nsk`, satisfies the signer requirement) | P1 | Token | **Confirmed not-expressible for a different reason** — same "destination must already exist and be Token-Program-owned" precondition as `RemoveLiquidity`'s finding above (`swap.rs` asserts `user_holding_b.account.program_owner == token_program_id` unconditionally) |
| Swap / AddLiquidity | `EXIST` | `amm_swap_into_existing_private_holding` | P2 | Token | Not started — swap into an *already-existing* private destination (`PrivateAuthorizedUpdate`); distinct from the two fresh-destination rows above, which are both confirmed not-expressible |
| NewDefinition | BASE | `amm_new_definition_private_initial_lp_holder` | P2 | Token | ✅ **Expressible**`new_definition.rs`'s own precondition on `user_holding_lp` (`account != Account::default() || is_authorized`) explicitly permits a fresh, authorized LP holder; confirmed working with a `PrivateAuthorizedInit` initial LP holder, unblocked by the clock fix above |
| NewDefinition | `EXIST` (negative), BASE | `amm_new_definition_private_unauthorized_lp_holder_is_not_expressible``PrivateUnauthorized` initial LP holder (`npk` only) | P2 | Token | **Confirmed not-expressible** — same guest-ABI signer requirement as the `Swap` `PrivateUnauthorized` finding above: `user_holding_lp` is `#[account(mut, signer)]` unconditionally, so it's rejected before `new_definition.rs`'s own "fresh LP holding requires user authorization" precondition is ever reached |
| Swap / AddLiquidity (vault) | `PDA` | `amm_swap_with_private_vault_pda` — predicted **not-expressible** per the ATA `PDA` finding (same `for_public_pda`-only root cause, confirmed in `amm_core`); write as a quick confirmation citing that finding, not a fresh investigation | P2 | Token | Not started — blocked by the separate PDA-formula finding, not the clock issue |
| AddLiquidity / RemoveLiquidity | `GROUP` | `amm_group_owned_lp_holding` | P3 | Token, `key_protocol` | Not started — no longer blocked by the clock issue (fixed above) |
| Pool/Config (any) | `PDA` | `amm_attempt_private_pool_pda` — same predicted not-expressible outcome as above; low priority given the vault row already confirms the root cause for this program | P3 | Token | Not started |
| Initialize, UpdateConfig, CreatePriceObservations, CreateOraclePriceAccount, SyncReserves | — | **Not planned** — admin/infra instructions over public protocol state; a private admin authority is legitimate but low value | — | — | Out of scope (for now) |
Note: every Swap/AddLiquidity/RemoveLiquidity chains to *both* Token (transfers) and TWAP
oracle (tick refresh) in one instruction — so every `CHAIN` row above is automatically also
a "some legs private, some public" test. Call that out explicitly when the test is written,
not as an incidental detail. **All of these are currently blocked by the circuit-level issue
above, since it fires with zero private accounts involved — no privacy dimension can be tested
on any pool-mutating AMM instruction until it's resolved.**
not as an incidental detail. **The clock-account blocker above is fixed (2026-07-15) — the
remaining unwritten rows are open to pick up, except the `PDA` rows, which are separately
blocked by the `for_public_pda`-only formula finding.**
---
File diff suppressed because it is too large Load Diff