dev: Allowlist (#625) basic-review follow-ups

Non-blocking follow-ups and one open question from the basic review of #625 (Allowlist module). None block the PR.

Each item below is the verbatim text of its inline review comment on #625, with a link to that comment.

Hardening

• contracts/src/security/Allowlist.compact:11-19

  🔵 followup: State the fail-closed lockout consequence explicitly. An empty allowlist rejects everyone, so a contract that enforces assertAllowed before the set is populated bricks access for all accounts (the operator included). The doc already mitigates this well with the eligibility-flag example, so this is one sentence of polish: note that the operator must populate and/or gate enforcement first. (Fails safe, so lower stakes than the inverse note on Blocklist.)

  added by claude (dev3-midnight-basic-review skill)

Docs

• contracts/src/security/Allowlist.compact:50

  🔵 followup: Add a one-line @notice/@dev on isAllowed (inherited by assertAllowed) that calling it discloses account to the public transcript. The module @notice covers the posture, but a dev reading only the circuit's API-ref entry sees @param account with no disclosure hint, and could leak an identifier when composing this into an otherwise-private contract.

  added by claude (dev3-midnight-basic-review skill)

Tests

• contracts/src/security/test/Allowlist.test.ts:112-125

  🔵 followup (test): This wiring test asserts an allow shows up in the raw Allowlist__allowed ledger, but never asserts a disallow removes it. Close the asymmetry so the state invariant is checked against the resulting ledger output (the field indexers subscribe to), not only via the isAllowed circuit:

  it('should reflect disallow in the public ledger', async () => {
    const sim = await AllowlistSimulator.create();
    await sim.allow(ALICE);
    expect((await sim.getPublicState()).Allowlist__allowed.member(ALICE)).toBe(true);
    await sim.disallow(ALICE);
    expect((await sim.getPublicState()).Allowlist__allowed.member(ALICE)).toBe(false);
  });
  

  added by claude (dev3-midnight-basic-review skill)

• contracts/src/security/test/Allowlist.test.ts:12

  🔵 followup (test): No test pins the all-zero Bytes<32> account. OZ Solidity special-cases address(0); this module deliberately does not, so a test guards against a future regression that adds an implicit exemption:

  const ZERO = new Uint8Array(32);
  it('should treat the all-zero account as an ordinary member', async () => {
    expect(await allowlist.isAllowed(ZERO)).toBe(false);
    await allowlist.allow(ZERO);
    expect(await allowlist.isAllowed(ZERO)).toBe(true);
    await allowlist.assertAllowed(ZERO);
    await allowlist.disallow(ZERO);
    await expect(allowlist.assertAllowed(ZERO)).rejects.toThrow('Allowlist: account not allowed');
  });
  

  added by claude (dev3-midnight-basic-review skill)

• contracts/src/security/test/Allowlist.test.ts:56-64

  🔵 followup (test): The allow→allow→disallow case is asserted only through isAllowed. Exercise the check seam (assertAllowed throwing) on the same path so both seams agree:

  it('should throw from assertAllowed after a single disallow of a multiply-allowed account', async () => {
    await allowlist.allow(ALICE);
    await allowlist.allow(ALICE);
    await allowlist.disallow(ALICE);
    await expect(allowlist.assertAllowed(ALICE)).rejects.toThrow('Allowlist: account not allowed');
  });
  

  added by claude (dev3-midnight-basic-review skill)

• contracts/src/security/test/Allowlist.test.ts:22-25

  🔵 followup (test): A view shouldn't mutate. Cheap assertion that isAllowed leaves the set untouched:

  it('should not mutate the set when querying isAllowed', async () => {
    const sim = await AllowlistSimulator.create();
    await sim.isAllowed(ALICE);
    expect((await sim.getPublicState()).Allowlist__allowed.member(ALICE)).toBe(false);
  });
  

  added by claude (dev3-midnight-basic-review skill)

Open questions

• contracts/src/security/Allowlist.compact:27

  ❔ question (non-blocking): Is security/ the final home, or should this and Blocklist move to accesscontrol/? Your own PR description flags it ("there is an argument that this belongs in accesscontrol/"). Worth deciding once for both modules so they stay together.

  added by claude (dev3-midnight-basic-review skill)

Source: basic review of #625. Sibling module follow-ups: #634 (Blocklist, #626).

Start with contracts/src/security/Allowlist.compact and contracts/src/security/test/Allowlist.test.ts, then run the existing Allowlist tests. Complete the documented hardening and test follow-ups, including ledger, zero-account, assertion, and non-mutating-view coverage. Before finishing, resolve whether Allowlist and Blocklist belong in security/ or accesscontrol/.