Propose composite policy interface (UNION/INTERSECT)#174
Conversation
Interface-only proposal for composite policies on IPolicyRegistry: - Extend PolicyType with UNION and INTERSECT (append-only; leaf discriminants 0/1 unchanged, gate rides the policy ID top byte). - Add createCompositePolicy(admin, gate, operands) and updateCompositeOperands(policyId, operands). Update is a full-set overwrite (replace-all), re-validated as at creation. - Add CompositePolicyCreated and CompositeOperandsUpdated events, and EmptyOperandSet / InvalidOperand errors. - Note on createPolicy/createPolicyWithAccounts that composite gates revert IncompatiblePolicyType. Operands are flat-only (leaf ALLOWLIST/BLOCKLIST), capped at the composite operand limit, and the set may not be empty. This changes only the interface; MockPolicyRegistry and the Rust precompile implementation follow in a separate PR, so the mock will not compile against this interface until then. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
Interface Coverage❌ Interface functions with no test coverage found. |
❌ Fork tests did not runThe build or setup step failed before any tests could execute. Check the workflow logs for details. |
| function createPolicy(address admin, PolicyType policyType) external returns (uint64 newPolicyId); | ||
|
|
||
| /// @notice Creates a new policy seeded with `accounts` as initial members. Permissionless. | ||
| /// @notice Creates a new leaf policy seeded with `accounts` as initial members. Permissionless. |
There was a problem hiding this comment.
Remove leaf here
Drop the "not an incremental edit" binary contrast and the "silently" adverb; make the flat-only note active. Wording only, no signature or behavior change. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
|
||
| /// @notice A composite policy (UNION or INTERSECT) was created over `operands`. | ||
| event CompositePolicyCreated( | ||
| uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operands |
There was a problem hiding this comment.
feel like I want to see the clarification of "policy" in there
| uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operands | |
| uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operandPolicies |
There was a problem hiding this comment.
Do we even need this event? Why not just use PolicyCreated as-is and then combine with the event below for declaring operand policies updated?
There was a problem hiding this comment.
I think we can use policy created. Only thing is I would assume a user want's an event that lists the new policy types, when a policy is created / updated.
| /// | ||
| /// @param policyId Composite policy to update. | ||
| /// @param operands Complete new operand set of existing leaf policy IDs. | ||
| function updateCompositeOperands(uint64 policyId, uint64[] calldata operands) external; |
There was a problem hiding this comment.
"composite operands" doesn't pass my gut check for being easily intuitable on surface. What other options for "operand" have we considered? My previous comments of expanding to "operand policy" may just be a bandaid for a name that needs reworking, although I'm okay still keeping it. In general, I don't think we should ever use the word "operand" without immediately following it with "policy" for clarity.
There was a problem hiding this comment.
From a CompSci PoV Operands seems okay but low level
From Maths - components / operands
From Layperson - "Part/Pieces"
"updateCompositeParts/Pieces" feels a lot more approachable if not a bit simple
There was a problem hiding this comment.
Agreed, I think childPolicyId's give more the intent
| error NoPendingAdmin(); | ||
|
|
||
| /// @notice A composite policy was created or updated with an empty operand set. A composite must | ||
| /// reference at least one operand, so the never-revert fold evaluator need not define |
There was a problem hiding this comment.
minimum two? since we're not offering a NOT gate, we need two at a minimum to be meaningful.
| /// Permissionless. A UNION authorizes an account if ANY operand authorizes it; an INTERSECT | ||
| /// authorizes only if EVERY operand does. `isAuthorized` folds the operands and never reverts. |
| /// @notice A composite policy's operand set was replaced in full with `operands`. The event | ||
| /// carries the complete post-update set, so an indexer reconstructs current state from a | ||
| /// single log with no delta folding. |
There was a problem hiding this comment.
the event carrying the full post-update set is nice devX. Would suggest moving the comment to its own line to emphasize (then slim down)
Co-authored-by: katzman <steve.katzman@coinbase.com>
Co-authored-by: Conner Swenberg <conner.swenberg@coinbase.com>
Co-authored-by: katzman <steve.katzman@coinbase.com>
Co-authored-by: Conner Swenberg <conner.swenberg@coinbase.com>
Proposal (interface-only)
Adds the composite-policy surface to
IPolicyRegistryas a design proposal. This PR changes only the interface — noMockPolicyRegistryor Rust precompile implementation — so the mock will not compile against it until a follow-up lands. Opened as a draft for design review.What changes
PolicyTypegainsUNIONandINTERSECT. Append-only: leaf discriminants (BLOCKLIST=0,ALLOWLIST=1) are unchanged, and the gate rides the policy ID top byte, so composites need no separate type storage.createCompositePolicy(address admin, PolicyType gate, uint64[] operands)— creates a UNION/INTERSECT over existing flat leaf policies (never other composites). Operands reference smaller, already-assigned IDs, so the reference graph is a DAG and cannot cycle.updateCompositeOperands(uint64 policyId, uint64[] operands)— replace-all: overwrites the whole operand set, re-validated exactly as at creation. The gate is fixed in the ID and cannot change.CompositePolicyCreated,CompositeOperandsUpdated(carries the full post-update set for single-log indexing).EmptyOperandSet,InvalidOperand(operandId).createPolicy/createPolicyWithAccountsnow notes they revertIncompatiblePolicyTypefor composite gates.Why replace-all for update
Operands live in a
uint64[](evaluation folds them in order). The registry has nouint64[]-mutation code today, so incremental add/remove would introduce the module's first swap-remove + length + dedup path. With the operand set capped small, sending the full list is the simpler design and reuses create's validation. Mirrors theIB20.updatePolicyfull-replace precedent over theupdateAllowlistincremental one.Open question for reviewers
The
CompositeOperandsUpdatedevent carries new operands only. A design council flagged anold → newshape (matchingupdatePolicy) as an option that closes a silent lost-update detection gap, at the cost of one warm SLOAD. Event surface is frozen at the fork, so this is worth settling before implementation.Follow-ups (not in this PR)
MockPolicyRegistryimplementation +test/unit/PolicyRegistry/tests (create, update, revert-order, fold truth tables).V2(versioned, new fork),composite_operandsstorage slot, dispatcher wiring.Generated with Claude Code