docs(BOP-509): document composite policy support in PolicyRegistry - #194
Open
rayyan224 wants to merge 3 commits into
Open
docs(BOP-509): document composite policy support in PolicyRegistry#194rayyan224 wants to merge 3 commits into
rayyan224 wants to merge 3 commits into
Conversation
The PolicyRegistry docs still described union/intersect composition as a future hardfork feature, but createCompositePolicy/updateComposite/ compositePolicyChildIds already ship in IPolicyRegistry. Bring the docs back in line with the interface and test suite: new UNION/INTERSECT policy type section, updated activation gating lists, and Create/Update Composite user flows with their revert conditions. Co-Authored-By: Claude <noreply@anthropic.com>
rayyan224
requested review from
amiecorso,
eric-ships,
ilikesymmetry and
stevieraykatz
as code owners
August 7, 2026 13:53
Interface Coverage✅ All interface functions have test coverage. |
📊 Forge Coverage (
|
| File | Lines | Stmts | Branches | Funcs |
|---|---|---|---|---|
| 🔴 B20FactoryLib.sol | 95.40% | 96.00% | 100.00% | 90.00% |
| 🔴 test/lib/ForceFeeder.sol | 0.00% | 0.00% | 100.00% | 0.00% |
| 🔴 test/lib/PrecompileProbe.sol | 0.00% | 0.00% | 0.00% | 0.00% |
| 🟢 MockActivationRegistry.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockActivationRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Asset.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟡 MockB20Factory.sol | 98.96% | 99.10% | 100.00% | 100.00% |
| 🟢 MockB20Stablecoin.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Storage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟡 MockPolicyRegistry.sol | 100.00% | 99.54% | 97.67% | 100.00% |
| 🟢 MockPolicyRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| Total | 96.79% | 97.28% | 98.14% | 96.49% |
Full report: download artifact. To browse locally: make coverage (runs forge coverage + genhtml + opens the HTML report).
rayyan224
commented
Aug 7, 2026
| - **`UNION`** (OR) — authorized if *any* child policy authorizes the account. | ||
| - **`INTERSECT`** (AND) — authorized only if *every* child policy authorizes the account. | ||
|
|
||
| A composite's child set is 2–4 existing simple (`ALLOWLIST`/`BLOCKLIST`) policy IDs — never another composite, and never a built-in sentinel (`ALWAYS_ALLOW`/`ALWAYS_BLOCK`). Composites reference their children live: `isAuthorized` reads current child membership on every call, not a snapshot taken at composite-creation time, so updating a child's membership immediately changes what the composite authorizes. |
Collaborator
Author
There was a problem hiding this comment.
We can just keep it as "Composites reference their children live: isAuthorized reads current child membership on every call. So updating a child's membership immediately changes what the composite authorizes."
rayyan224
commented
Aug 7, 2026
| PolicyRegistry-->>PolicyAdmin: emit CompositePolicyUpdated(policyId, updater, childPolicyIds) | ||
| ``` | ||
|
|
||
| `childPolicyIds` is a full replacement, not a merge — a child omitted from the new set no longer governs the composite, even if it governed before. There is no clear-the-list path: the new set must still satisfy the same size and child-validity rules as creation. Because composites read child membership live, `isAuthorized` reflects the new set immediately. |
Collaborator
Author
There was a problem hiding this comment.
Replace with
"childPolicyIds is a full replacement, a child omitted from the new set no longer governs the composite. The new set must still satisfy the same size and child-validity rules as creation"
Co-Authored-By: Claude <noreply@anthropic.com>
✅ Fork tests: all 711 passedbase/base is fully in sync with the base-std spec. |
| - **`UNION`** (OR) — authorized if *any* child policy authorizes the account. | ||
| - **`INTERSECT`** (AND) — authorized only if *every* child policy authorizes the account. | ||
|
|
||
| A composite's child set is 2–4 existing simple (`ALLOWLIST`/`BLOCKLIST`) policy IDs — never another composite, and never a built-in sentinel (`ALWAYS_ALLOW`/`ALWAYS_BLOCK`). Composites reference their children live: `isAuthorized` reads current child membership on every call. So updating a child's membership immediately changes what the composite authorizes. |
Member
There was a problem hiding this comment.
I wasn't aware we were excluding the sentinels (though that makes sense). I think the interface doc is missing this nuance. From createCompositePolicy in IPolicyRegistry L124
/// @dev Child policies must be simple policies (ALLOWLIST or BLOCKLIST), never another composite.
/// The child-policy set is capped at 4.
/// @dev Reverts with `IncompatiblePolicyType` when `policyType` is not UNION or INTERSECT.
/// @dev Reverts with `ZeroAddress` when `admin` is `address(0)`.
/// @dev Reverts with `ChildPoliciesOutsideOfRange` when `childPolicyIds.length` is not in
/// `[MIN_COMPOSITE_CHILD_POLICIES, MAX_COMPOSITE_CHILD_POLICIES]`.
/// @dev Reverts with `PolicyNotFound` when any child policy does not exist.
/// @dev Reverts with `InvalidChildPolicy` when any child policy is not a simple policy or a built-in policy.
/// @dev Panics with arithmetic overflow (Panic 0x11) when the policy counter has reached its maximum value.
///
Collaborator
Author
There was a problem hiding this comment.
Good catch — the top-level @dev line only called out "never another composite" and left the sentinel exclusion to be inferred from the later revert line. Fixed the NatSpec on both createCompositePolicy and updateComposite to spell out the built-in-sentinel exclusion explicitly, matching the revert conditions and the README.
createCompositePolicy/updateComposite already revert InvalidChildPolicy for ALWAYS_ALLOW/ALWAYS_BLOCK children, but the leading @dev lines only called out "not another composite" and left the sentinel case implicit. Spell it out in both, matching the behavior in the composite test suite and the PolicyRegistry docs. Co-Authored-By: Claude <noreply@anthropic.com>
stevieraykatz
approved these changes
Aug 7, 2026
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
2 participants
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.
Summary
docs/PolicyRegistry/README.mdstill describedUNION/INTERSECTcomposition as a "planned for a future hardfork" feature, butcreateCompositePolicy,updateComposite, andcompositePolicyChildIdsalready ship inIPolicyRegistry(seesrc/interfaces/IPolicyRegistry.sol,test/unit/PolicyRegistry/*Composite*.t.sol).compositePolicyChildIds,MIN_COMPOSITE_CHILD_POLICIES,MAX_COMPOSITE_CHILD_POLICIESto the always-callable list, andcreateCompositePolicy/updateCompositeto the gated-writes list.Source: BOP-509 — flagged by @connerswenberg in Slack as the docs missing composite policy coverage. The ticket also floats a doc/src-diff-detecting harness as a follow-up; scoping that separately rather than bundling it into this docs fix.
Test plan
src/interfaces/IPolicyRegistry.solandtest/unit/PolicyRegistry/{createCompositePolicy,updateComposite,compositeChildPolicyLimits,compositePolicyChildIds,isAuthorized}.t.sol.