test(privacy): add privacy-preserving coverage, ported to lez v0.2.4 - #350
Open
0x-r4bbit wants to merge 1 commit into
Open
test(privacy): add privacy-preserving coverage, ported to lez v0.2.4#3500x-r4bbit wants to merge 1 commit into
0x-r4bbit wants to merge 1 commit into
Conversation
Rebase of #215 onto main. Adds privacy-preserving integration-test coverage (token, ata, stablecoin, amm) exercising LEE's private-account variants (REGULAR/EXIST/GROUP/PDA/CHAIN) plus docs/findings.md. Ported from logos-execution-zone v0.2.0 to the v0.2.4 API that main pins: - nssa/nssa_core -> lee/lee_core; clock_core and key_protocol at v0.2.4. - PrivateUnauthorized -> PrivateForeignInit; drop epk/ssk/encapsulate_deterministic; identities now carry vpk/random_seed/commitment_root. - Fresh private-init pre-states are is_authorized = true (upstream #621). - Message::try_from_circuit_output(ids, nonces, out) -> from_circuit_output(nonces, out). - AccountId::for_regular_private_account gains the vpk argument. - Stablecoin Position debt model (normalized_debt_amount + position_nonce) and amm SwapExact* field sets updated to the current program APIs. The negative tests the report anticipated #621/#625 would resolve are reconciled with v0.2.4: three are now passing positive tests (token_initialize_private_account_via_foreign_init, token_new_fungible_definition_foreign_init_holder, amm_new_definition_foreign_init_lp_holder); the amm swap-to-fresh-destination case stays negative but now blocks on AMM's destination-must-exist precondition. docs/findings.md updated accordingly. All 112 integration tests pass (RISC0_DEV_MODE=1); make clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0x-r4bbit
requested review from
3esmit and
gravityblast
and
a lite review from Copilot
September 3, 2026 15:54
There was a problem hiding this comment.
🟡 Changes recommended
Documentation/comments still use pre-v0.2.4 PrivateUnauthorized terminology and contain a few correctness/clarity issues that should be aligned to PrivateForeignInit and cleaned up before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a substantial suite of privacy-preserving integration tests for LEZ programs (token/ata/stablecoin/amm) against the v0.2.4 lee/lee_core APIs, plus a findings write-up documenting which privacy patterns are expressible vs. blocked.
Changes:
- Introduces shared helpers for building
InputAccountIdentityvariants and for group-owned (GMS-based) private-account test setup. - Expands integration tests across Token/ATA/Stablecoin/AMM to cover REGULAR/EXIST/GROUP/CHAIN privacy patterns (including several negative “not expressible” cases).
- Adds
docs/findings.mddescribing outcomes and linking them to upstream LEZ protocol constraints/changes (e.g., PR #621/#625).
File summaries
| File | Description |
|---|---|
| programs/integration_tests/tests/token.rs | Refactors fixtures and adds many privacy-preserving Token tests (foreign-init, authorized init/update, group-owned flows). |
| programs/integration_tests/tests/stablecoin.rs | Adds privacy-preserving Stablecoin tests (withdraw to private/group destinations, repay from private holdings, and a documented non-expressible case). |
| programs/integration_tests/tests/ata.rs | Adds privacy tests demonstrating private owners/signers, transfers to existing private recipients, and a private-PDA ATA negative test. |
| programs/integration_tests/tests/amm.rs | Adds privacy tests for swaps/liquidity with private holdings, and adjusts clock ownership to satisfy v0.2.4 circuit/output invariants. |
| programs/integration_tests/src/lib.rs | New shared helpers for privacy identities plus group-owner/member key distribution via seal/unseal. |
| programs/integration_tests/Cargo.toml | Adds key_protocol dependency (v0.2.4) to support group-key tests. |
| docs/findings.md | New report summarizing privacy coverage results and open items. |
Review details
Suppressed comments (7)
docs/findings.md:30
- The summary table uses the old
PrivateUnauthorizedname; for v0.2.4 this should bePrivateForeignInit(formerlyPrivateUnauthorized).
| `PrivateUnauthorized`| ❌ | anyone |
docs/findings.md:71
- The privacy-coverage matrix still refers to
PrivateUnauthorized, but v0.2.4 usesPrivateForeignInitfor npk-only initialization.
| EXIST | private account initialized without `nsk`; `PrivateUnauthorized` |
docs/findings.md:106
- This table row still uses the pre-v0.2.4
PrivateUnauthorizedname; replace withPrivateForeignInitto match the actual identity variant used by the tests.
| Create | `ata_create_from_private_owner` | REGULAR, EXIST | Any third party can bootstrap another owner's ATA using only that owner's public key material (`PrivateUnauthorized` — `npk`/`vpk` only, no `nsk`) — `Create` never asserts `owner.is_authorized` | ✅ |
docs/findings.md:116
- Terminology + spacing:
PrivateUnauthorizedwas renamed toPrivateForeignInitin v0.2.4, and the missing space after the backtick makes the sentence harder to read.
- Transfer explicitly blocks `PrivateUnauthorized`and `PrivateAuthorizedInit`. ATA's transfer checks that the recipient's account is non-default. E.g., ATA can not transfer funds to a third-party's private account.
docs/findings.md:131
- This Stablecoin remark still refers to
PrivateUnauthorized; on v0.2.4 the npk-only initialization variant isPrivateForeignInit.
- `WithdrawCollateral` does not support withdrawals to `PrivateUnauthorized` and `PrivateAuthorizedInit`; explicitly checks that the destination account is not default. Demonstrated with the test `stablecoin_withdraw_collateral_to_new_private_destination_is_not_expressible`. **[Open — Programs]**
docs/findings.md:139
- These Token table rows still use the pre-v0.2.4
PrivateUnauthorizedname; usePrivateForeignInitfor the npk-only initialization variant.
| Transfer | `token_shielded_transfer` | EXIST | A public sender shields tokens into a fresh private recipient (`PrivateUnauthorized` — only `npk`/`vpk` known, no `nsk`) | ✅ |
| Transfer | `token_private_transfer` | REGULAR -> EXIST | Two private accounts (sender via `PrivateAuthorizedUpdate` + fresh recipient via `PrivateUnauthorized`) compose in a single transaction with no public account at all — fulfills the "multiple private accounts in one tx" | ✅ |
docs/findings.md:145
- These rows also use
PrivateUnauthorizedterminology; in v0.2.4 the correct name isPrivateForeignInit.
| Transfer | `token_shielded_transfer_authorized_private_init` | REGULAR | Fresh recipient self-initializes via `PrivateAuthorizedInit` (own `nsk` supplied) instead of being passively credited via `PrivateUnauthorized` | ✅ |
| Transfer | `token_transfer_into_existing_private_holding` | REGULAR | Similar to `token_shielded_transfer_authorized_private_init`, but this shielded transaction does not initialize the private account. Second transfer into an already-shielded recipient — confirms crediting an existing private account requires the recipient's own cooperation (`nsk`), not just their public key | ✅ |
| Transfer | `token_private_transfer_into_existing_private_holding` | REGULAR -> REGULAR | Both legs private (sender + recipient) in one transaction, and the recipient is already existing rather than fresh | ✅ |
| Transfer | `token_group_owned_holding_shared_control_transfer` | GROUP -> EXIST | Group-owned sender (real GMS seal/unseal handshake) spends outward via `Transfer` to a fresh private recipient (`PrivateUnauthorized`) | ✅ |
| Mint | `token_mint_private_unauthorized` | EXIST | Mint directly to a fresh private recipient (self-authority signer + `PrivateUnauthorized` recipient) | ✅ |
- Files reviewed: 7/8 changed files
- Comments generated: 10
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| # Privacy coverage in LEZ programs | ||
|
|
||
| LEZ programs, ideally, are privacy agnostic. E.g., a program should work the same for public and private accounts. Currently, LEZ program integration tests only cover public accounts. This task, we expand the tests for LEZ programs to determine how compatible LEZ programs are with selective privacy. |
Comment on lines
+14
to
+18
| Regular private accounts can be initialized with or without knowledge of the account's nullifier secret key `nsk`. This results in two initialization "types": `PrivateUnauthorized` and `PrivateAuthorizedInit`. | ||
|
|
||
| - `PrivateUnauthorized` | ||
|
|
||
| A special case for private accounts initialization that uses only public keys `npk` and `vpk`. Example: Alice can use Bob's keys (`npk`, `vpk`) and an `identifier` to send Bob a private transaction. Since Alice does not know the corresponding `nsk`, she is unable to spend the resulting private account. E.g., Alice cannot authorize the transaction. |
| A special case for private accounts initialization that uses only public keys `npk` and `vpk`. Example: Alice can use Bob's keys (`npk`, `vpk`) and an `identifier` to send Bob a private transaction. Since Alice does not know the corresponding `nsk`, she is unable to spend the resulting private account. E.g., Alice cannot authorize the transaction. | ||
|
|
||
| - `PrivateAuthorizedInit` | ||
| Private account initialized using the account's `nsk` (and some `identifier`). This operation cannot be done by the a third-party (an entity that does not possess spending authority of the account). |
|
|
||
| ## Private PDA | ||
|
|
||
| Private PDAs spending is restrict by a specific program. E.g., an AMM pool has PDAs for liquidity definition and vaults (for Token A and Token B). A program sets `is_authorized = true` for an account (purported PDA) by checking the correctness of its `AccountId`. |
| > from the #621 signer check to AMM's own destination-must-already-exist precondition **[Open — | ||
| > Programs]**. | ||
|
|
||
| Privacy coverage for LEZ program tests is greatly improved from the added tests. Though, there are a few noticable gaps: |
| - Pre-#621, `PrivateUnauthorized` accounts were blocked by programs with a check requiring `is_authorized = true`, since a fresh `PrivateUnauthorized` account was always initialized with `is_authorized = false`. This was a `logos-execution-zone` protocol-level issue, not something programs could work around. It has been resolved by [PR 621](https://github.com/logos-blockchain/logos-execution-zone/pull/621) (in v0.2.4): the variant is now `PrivateForeignInit` and its pre-state is authorized; see the Action items section. | ||
| - Privacy transactions have issues with chain calls in which multiple calls affect the same private account. The privacy preserving circuit's `authorized_accounts` bookkeeping is monotonic (once an account is authorized, every later occurrence within the same transaction must also declare it authorized), which rejects some call patterns that are valid on the public-transaction path (E.g. `Stablecoin::OpenPosition`). This is a `logos-execution-zone` protocol level issue. A proposed revision to account updates would mitigate this issue: accounts updated iteratively based on their state diff rather than "full replacement". | ||
|
|
||
| Additionally, testing undercovered a "bug" in LEZ: |
Comment on lines
+773
to
+776
| /// `WithdrawCollateral` blocks withdraws to private accounts (via private donations); | ||
| /// `PrivateUnauthorized` account initialization (e.g., `nsk` is not known) is not permitted | ||
| /// due to the assertion in `withdraw_collateral.rs` asserts `destination.account != | ||
| /// Account::default()` |
Comment on lines
+674
to
+675
| /// Shielded transaction to a private account using the account's `nsk`. | ||
| /// `token_shielded_transfer` only uses the account's `npk`; thus, `PrivateUnauthorized` private. |
Comment on lines
+854
to
+855
| /// Mints directly to a new recipient private holding (`PrivateUnauthorized`). | ||
| /// The recipient's cooperation is unnecessary; only known of the recipient's `npk`, `vpk`. |
Comment on lines
+1305
to
+1308
| /// Initializes a private holding account directly (private account secret keys are known). | ||
| /// `InitializeAccount` requires `account_to_initialize` to be authorized. E.g., for private | ||
| /// accounts must be `PrivateAuthorizedInit` and not `PrivateUnauthorized`; the account owner | ||
| /// must supply their own `nsk`. |
4 tasks
3esmit
requested changes
Sep 4, 2026
3esmit
left a comment
3esmit
left a comment
Collaborator
There was a problem hiding this comment.
Two negative fixtures use invalid v0.2.4 PrivateForeignInit pre-states, so current program errors mask a second circuit rejection. Both positive private-input AMM swap tests also carry an unnecessary output-recipient signature.
| let position_pre = | ||
| AccountWithMetadata::new(state.get_account_by_id(position_id), false, position_id); | ||
| let vault_pre = AccountWithMetadata::new(state.get_account_by_id(vault_id), false, vault_id); | ||
| let destination_pre = AccountWithMetadata::new(Account::default(), false, destination_id); |
Collaborator
There was a problem hiding this comment.
Keep this foreign-init pre-state authorized
private_foreign_init_identity maps to v0.2.4 PrivateForeignInit, whose circuit branch requires pre_state.is_authorized == true. This test currently stops earlier at Destination must be initialized, so false is masked. Once that program guard is fixed, the test will still fail in the privacy circuit instead of exposing the newly supported path. Set this flag to true; the intended Stablecoin error still fires with that valid pre-state.
| false, | ||
| Ids::token_lp_definition(), | ||
| ); | ||
| let user_a_pre = AccountWithMetadata::new(Account::default(), false, user_a_id); |
Collaborator
There was a problem hiding this comment.
Keep both foreign-init pre-states authorized
Both slots are paired with private_foreign_init_identity, but v0.2.4 rejects a PrivateForeignInit whose pre-state is unauthorized. The current Token A ownership panic fires first and hides that second failure. If the documented AMM destination guard is removed, this test therefore remains negative for the wrong reason. Set both flags to true; the focused test still reaches the intended AMM ownership error.
| Ids::vault_b(), | ||
| ); | ||
| let user_a_pre = AccountWithMetadata::new(user_a_account, true, user_a_id); | ||
| let user_b_pre = |
Collaborator
There was a problem hiding this comment.
Exercise the swap without an output-recipient signature
swap_exact_input declares only user_input_holding as signer, but this test authorizes user_b, supplies its nonce/key, and expects a signer nonce bump. That extra witness lets the privacy test pass even if output-recipient cooperation becomes required. Mark user_b unauthorized and use empty public nonces/witness keys; the test passes with that minimal signer set. The same redundant signature appears in amm_swap_exact_output_private_user_holding.
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.
Rebase of #215 onto main. Adds privacy-preserving integration-test coverage (token, ata, stablecoin, amm) exercising LEE's private-account variants (REGULAR/EXIST/GROUP/PDA/CHAIN) plus docs/findings.md.
Ported from logos-execution-zone v0.2.0 to the v0.2.4 API that main pins:
The negative tests the report anticipated #621/#625 would resolve are reconciled with v0.2.4: three are now passing positive tests
(token_initialize_private_account_via_foreign_init, token_new_fungible_definition_foreign_init_holder, amm_new_definition_foreign_init_lp_holder); the amm swap-to-fresh-destination case stays negative but now blocks on AMM's destination-must-exist precondition. docs/findings.md updated accordingly.