Skip to content

Correctness fixes on top of policy-registry impl#11

Draft
amiecorso wants to merge 3 commits into
impl/policy-registryfrom
amie/correctness-fixes
Draft

Correctness fixes on top of policy-registry impl#11
amiecorso wants to merge 3 commits into
impl/policy-registryfrom
amie/correctness-fixes

Conversation

@amiecorso
Copy link
Copy Markdown
Collaborator

Summary

Three atomic commits on top of impl/policy-registry from a review pass on
the reference implementation. Scope is correctness only — no style cleanup,
no tests (separate pass). Each commit is independently reviewable; build
clean throughout.

Commits

1. Pack constituent type bit in compound slot for 2-SLOAD hot path (c816cac)

The original _checkRole docstring claimed "at most 2 SLOADs for any policy"
but the implementation actually did 3 for compound: compound slot +
constituent slot (loaded only to read its type byte) + member set. Reduces
each compound constituent ID from 62 to 61 bits and uses the freed bit per
constituent to cache whitelist-vs-blacklist type at compound creation. Hot
path now reads only the compound slot + the relevant member set, matching
the docstring claim. Every B-20 transfer pays this path on every call, so
the saved SLOAD compounds at protocol scale.

2. Bound policy ID counter to 61-bit packable range (d1bba93)

The interface exposes uint64 IDs but compound slots only fit 61 bits per
constituent. _nextPolicyId now reverts with PolicyIdOverflow before
issuing an ID that would silently truncate when packed. Practically
unreachable (counter would need to reach 2^61 ≈ 2.3e18) but the alternative
was an unchecked truncation hazard hidden behind a comment. Halt-on-overflow
is preferable to silent-corruption-on-overflow.

3. Two-step admin transfer + one-way freezePolicy (91faa39)

Replaces single-step setPolicyAdmin (typo bricks a sanctions list) with
the OZ Ownable2Step shape: beginPolicyAdminTransfer /
acceptPolicyAdminTransfer / cancelPolicyAdminTransfer +
pendingPolicyAdmin view. Adds freezePolicy as a one-way switch that
permanently locks both membership and admin. The frozen flag lives in
bit 168 of the simple-policy slot (above the 160-bit admin, in
previously-unused space).

Freeze is a separate concept from admin renunciation because setting admin = address(0) would break the existence sentinel: a WHITELIST policy (type
byte = 0) with admin == address(0) would pack to literally 0,
indistinguishable from "never created."

Design choices worth a second look

  • _requireConstituent returns a PolicyType placeholder (WHITELIST)
    for built-ins.
    Inert because the hot path short-circuits on built-in ID
    before consulting type, but a subtle coupling — if anyone refactors
    _checkRole and drops the short-circuit, the placeholder becomes a real
    bug. Documented inline.

  • freezePolicy is one-way and cannot be unfrozen. Considered making it
    reversible but "frozen but unfreezable" isn't a meaningful guarantee for
    downstream integrators.

  • acceptPolicyAdminTransfer preserves the frozen bit defensively.
    Currently dead code (the freeze check short-circuits earlier) but kept in
    case the semantics ever change to allow rotation while frozen.

  • beginPolicyAdminTransfer(id, address(0)) silently clears the pending
    slot.
    A second cancellation path; could be tightened by requiring
    nonzero newAdmin and forcing callers to use cancelPolicyAdminTransfer.

  • cancelPolicyAdminTransfer reverts with NoTransferPending when there's
    no pending admin.
    Alternative is silent no-op; went with revert because
    cancelling nothing suggests a caller mental-model bug worth surfacing.

Out of scope

  • Style cleanups (named arguments at multi-arg call sites, pragma pinning).
  • Tests.
  • The createPolicyWithAccounts dedup observation — not a bug, sloppy on
    caller's part only.
  • isAuthorized SLOAD dedup — modest cleanup, not in this PR's scope.

amiecorso added 3 commits May 17, 2026 08:26
Each compound constituent field now carries both the constituent policy ID
(61 bits) and a single type bit (0=whitelist/built-in, 1=blacklist) in 62
bits total. _checkRole on a compound policy now reads only:
  1. the compound slot itself
  2. the relevant constituent's member set
The constituent's own policy slot is no longer loaded on the hot path,
matching the 'exactly 2 SLOADs per check' claim that the previous
implementation overstated.
The compound-policy packing format reserves 61 bits per constituent ID
(2.3e18 IDs), but the public API exposes uint64 IDs. _nextPolicyId now
reverts with PolicyIdOverflow before issuing an ID that would silently
truncate when stored in a compound slot. This enforces the packing
invariant at the source: every ID returned from a create function is
guaranteed safe to round-trip through encodeField / decodeField, so
downstream code can rely on it without per-call defensive checks.

Practically unreachable today (counter starts at 2 and only ever
increments by 1), but the alternative is an unchecked truncation hazard
hidden behind a comment.
Replaces single-step setPolicyAdmin (typo bricks a sanctions list) with
the OZ Ownable2Step shape:
  - beginPolicyAdminTransfer(policyId, newAdmin) records a pending admin
  - acceptPolicyAdminTransfer(policyId)          callable by pending only
  - cancelPolicyAdminTransfer(policyId)          aborts the in-flight transfer
  - pendingPolicyAdmin(policyId) view            inspects current pending

Adds freezePolicy(policyId) as a one-way switch that locks the policy's
membership AND admin permanently:
  - subsequent modifyPolicy* calls revert with PolicyFrozen
  - subsequent admin transfers revert with PolicyFrozen
  - isPolicyFrozen(policyId) view exposes the state

Frozen flag lives in bit 168 of the policy slot (currently unused space
above the 160-bit admin). Compound policies have no admin and cannot be
frozen; calls revert with IncompatiblePolicyType.

Why a freeze flag instead of letting admin be set to address(0):
the existence sentinel _policyData[id] == 0 means 'never created'. A
WHITELIST policy (type byte = 0) with admin = address(0) would pack to
0 exactly, colliding with the sentinel. The frozen bit avoids that
collision and gives a cleaner one-way switch with a distinct event.

Pending admin lives in its own mapping rather than the packed slot so
the authorization hot path isn't affected; rotation is cold so the
extra accept-time SLOAD is fine.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant