Skip to content

Propose composite policy interface (UNION/INTERSECT)#174

Draft
rayyan224 wants to merge 8 commits into
mainfrom
composite-policy-interface-proposal
Draft

Propose composite policy interface (UNION/INTERSECT)#174
rayyan224 wants to merge 8 commits into
mainfrom
composite-policy-interface-proposal

Conversation

@rayyan224

Copy link
Copy Markdown
Collaborator

Proposal (interface-only)

Adds the composite-policy surface to IPolicyRegistry as a design proposal. This PR changes only the interface — no MockPolicyRegistry or 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

  • PolicyType gains UNION and INTERSECT. 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.
  • Events: CompositePolicyCreated, CompositeOperandsUpdated (carries the full post-update set for single-log indexing).
  • Errors: EmptyOperandSet, InvalidOperand(operandId).
  • NatSpec on createPolicy / createPolicyWithAccounts now notes they revert IncompatiblePolicyType for composite gates.

Why replace-all for update

Operands live in a uint64[] (evaluation folds them in order). The registry has no uint64[]-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 the IB20.updatePolicy full-replace precedent over the updateAllowlist incremental one.

Open question for reviewers

The CompositeOperandsUpdated event carries new operands only. A design council flagged an old → new shape (matching updatePolicy) 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)

  • MockPolicyRegistry implementation + test/unit/PolicyRegistry/ tests (create, update, revert-order, fold truth tables).
  • Rust precompile V2 (versioned, new fork), composite_operands storage slot, dispatcher wiring.

Generated with Claude Code

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>
@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

Interface Coverage

❌ Interface functions with no test coverage found.

Warning: Found unknown `base` config for profile `fork` defined in foundry.toml.
Warning: optimizer settings and `viaIR` have been disabled for accurate coverage reports.
If you encounter "stack too deep" errors, consider using `--ir-minimum` which enables `viaIR` with minimum optimization resolving most of the errors.
See more: https://book.getfoundry.sh/guides/best-practices/stack-too-deep
Error: Compiler run failed:
Error (3881): Documented parameter "gate" not found in the parameter list of the function.
   --> src/interfaces/IPolicyRegistry.sol:137:5:
    |
137 |     /// @notice Creates a new composite policy that combines existing membership policies under a logic
    |     ^ (Relevant source part starts here and spans across multiple lines).

Error (3656): Contract "MockPolicyRegistry" should be marked as abstract.
  --> test/lib/mocks/MockPolicyRegistry.sol:57:1:
   |
57 | contract MockPolicyRegistry is IPolicyRegistry {
   | ^ (Relevant source part starts here and spans across multiple lines).
Note: Missing implementation: 
   --> src/interfaces/IPolicyRegistry.sol:159:5:
    |
159 |     function createCompositePolicy(address admin, PolicyType policyType, uint64[] calldata childPolicyIds)
    |     ^ (Relevant source part starts here and spans across multiple lines).
Note: Missing implementation: 
   --> src/interfaces/IPolicyRegistry.sol:243:5:
    |
243 |     function updateCompositeChildren(uint64 policyId, uint64[] calldata childPolicyIds) external;
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Warning (1699): "selfdestruct" has been deprecated. Note that, starting from the Cancun hard fork, the underlying opcode no longer deletes the code and data associated with an account and only transfers its Ether to the beneficiary, unless executed in the same transaction in which the contract was created (see EIP-6780). Any use in newly deployed contracts is strongly discouraged even if the new behavior is taken into account. Future changes to the EVM might further reduce the functionality of the opcode.
  --> test/lib/ForceFeeder.sol:20:13:
   |
20 |             selfdestruct(target)
   |             ^^^^^^^^^^^^\n```

@github-actions

github-actions Bot commented Jul 22, 2026

Copy link
Copy Markdown

❌ Fork tests did not run

The build or setup step failed before any tests could execute. Check the workflow logs for details.

Comment thread src/interfaces/IPolicyRegistry.sol Outdated
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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment thread src/interfaces/IPolicyRegistry.sol Outdated

/// @notice A composite policy (UNION or INTERSECT) was created over `operands`.
event CompositePolicyCreated(
uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operands

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel like I want to see the clarification of "policy" in there

Suggested change
uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operands
uint64 indexed policyId, address indexed creator, PolicyType policyType, uint64[] operandPolicies

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment thread src/interfaces/IPolicyRegistry.sol Outdated
///
/// @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;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think childPolicyId's give more the intent

Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment thread src/interfaces/IPolicyRegistry.sol Outdated
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

@stevieraykatz stevieraykatz Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minimum two? since we're not offering a NOT gate, we need two at a minimum to be meaningful.

Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment on lines +132 to +133
/// 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove fluff or move to @dev block. @notice should be one line generally.

Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment thread src/interfaces/IPolicyRegistry.sol Outdated
Comment on lines +92 to +94
/// @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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

rayyan224 and others added 6 commits July 23, 2026 10:08
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>
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.

4 participants