feat(policy): add compositePolicyChildIds view for composite child sets - #183
Merged
Conversation
Composite policies (UNION/INTERSECT) had no on-chain read path for their
child set. The only ways to recover it were decoding the
CompositePolicyUpdated event or a raw eth_getStorageAt against slot 4.
Adds a total view returning the stored child set:
function compositePolicyChildIds(uint64 policyId)
external view returns (uint64[] memory);
Semantics follow the existing read surface, which documents "Never
reverts" on every getter: simple policies, built-in sentinels, unknown
IDs, and malformed IDs all return an empty array rather than reverting
with IncompatiblePolicyType. An empty return is unambiguous in practice
because both write paths reject a set outside [2, 4], so a composite
that exists always holds at least two children and there is no
clear-the-list path.
Naming: childPolicyIds was the first choice for symmetry with the write
path, but a function of that name collides with the childPolicyIds
parameter on createCompositePolicy and updateComposite, emitting solc
warning 8760 in MockPolicyRegistry. compositePolicyChildIds compiles
clean and keeps "composite" at the call site, which reads better given
that a non-composite returns empty.
The mock's well-formedness guard must precede _isComposite: that helper
routes through _typeOf, whose PolicyType(uint8(...)) conversion panics
with 0x21 on a type byte above INTERSECT. The Rust precompile compares
raw type bytes and is safe without the guard.
Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
rayyan224
requested review from
amiecorso,
eric-ships,
ilikesymmetry and
stevieraykatz
as code owners
July 29, 2026 21: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% | 100.00% | 100.00% | 100.00% |
| 🟢 MockPolicyRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| Total | 96.74% | 97.34% | 98.68% | 96.46% |
Full report: download artifact. To browse locally: make coverage (runs forge coverage + genhtml + opens the HTML report).
|
Comment on lines
+120
to
+123
| /// @notice Verifies an existing composite never reports fewer than MIN_CHILD_POLICIES children | ||
| /// @dev This is what makes an empty return unambiguously mean "not a composite": there is no | ||
| /// clear-the-list path, so a created composite can never decay to an empty set. | ||
| function test_compositePolicyChildIds_success_createdCompositeIsNeverEmpty() public { |
Member
There was a problem hiding this comment.
Weird to me that we call this a success test when this is the only instance of testing the revert case for ChildPoliciesOutsideOfRange.
Co-authored-by: katzman <steve.katzman@coinbase.com>
Co-authored-by: katzman <steve.katzman@coinbase.com>
Addresses review feedback on #183. test_..._success_createdCompositeIsNeverEmpty asserted a ChildPoliciesOutsideOfRange revert under a _success_ name, against the repo's _success_/_revert_ convention, and duplicated coverage already pinned thoroughly by updateComposite.t.sol (under-range 0 and 1, over-range 5..8) plus three other files. Scopes the file to the selector's own behavior, 10 tests -> 5: - returnsCreationSet newly created - tracksUpdateComposite exists and updated - emptyForUncreated does not exist - emptyAfterFailedCreation creation failed, nothing persisted (new) - emptyForMalformedId malformed id (regression guard) The new failed-creation test uses a bare vm.expectRevert() rather than the typed selector: the failure is setup for the read, not the assertion under test, which is what made the old test read as a revert test in disguise. It also pins that the predicted ID is the one the failed call would have taken, by showing the next valid creation claims exactly it — otherwise the empty read would pass for free on an unrelated ID. Dropped matchesEmittedEvent, preservesDuplicates, survivesRenounce, emptyForSimplePolicy, emptyForBuiltins, liveCompositeIsNeverEmpty. Guard coverage survives: emptyForUncreated fuzzes a top byte across the whole PolicyType range, so it exercises the !_isComposite branch that emptyForSimplePolicy covered, and emptyForMalformedId still covers !_isWellFormed. Also applies the two natspec suggestions from review. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
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
3 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
Composite policies (UNION/INTERSECT) shipped in #174 with no on-chain read path for their child set. The only ways to recover it today are decoding the
CompositePolicyUpdatedevent or a raweth_getStorageAtagainst slot 4 — the smoke harness does the former (chain.py: composite_children_from).This adds the missing getter to
IPolicyRegistry, implements it inMockPolicyRegistry, and covers it with unit tests.Semantics
Total — it never reverts, matching the contract every existing getter documents explicitly (
policyExists,policyAdmin,pendingPolicyAdminall say "Never reverts" and degrade to a zero value). An empty array is the array-typed analogue ofaddress(0):[]ALWAYS_ALLOW/ALWAYS_BLOCK)[][][]Returning
[]rather than revertingIncompatiblePolicyTypeon a non-composite is safe because an empty return is unambiguous in practice: both write paths reject a set outside[2, 4]withChildPoliciesOutsideOfRange, and there is no clear-the-list path, so a composite that exists always holds at least two children. There's a test pinning that invariant so it can't silently regress.Ordering is preserved verbatim; the registry neither sorts nor de-duplicates, so a
UNIONover[a, a]reads back as[a, a].On the name
childPolicyIdswas the first choice, for exact symmetry with the write path and the event field. It doesn't work: a function of that name collides with thechildPolicyIdsparameter oncreateCompositePolicyandupdateComposite, and solc emits warning 8760 ("This declaration has the same name as another declaration") inMockPolicyRegistry. Verified against solc 0.8.30 —compositePolicyChildIdscompiles clean. The only alternative fix was renaming the mock's parameters away from the interface's, which destroys the symmetry that motivated the name in the first place.It also reads better at the call site given that a non-composite returns empty.
Reviewer note
In the mock, the
_isWellFormedguard must precede_isComposite. That helper routes through_typeOf, whosePolicyType(uint8(...))conversion panics with0x21on a type byte aboveINTERSECT. The two guards look collapsible but aren't. The Rust precompile compares raw type bytes, so it's safe without the guard — this is a Solidity-side hazard only. Caught bytest_compositePolicyChildIds_success_emptyForMalformedId, which failed before the guard was added.Testing
forge test— 136 passed, 0 failed, 4 skipped.forge fmt --checkclean. 10 new tests intest/unit/PolicyRegistry/compositePolicyChildIds.t.solcovering creation-order readback,updateCompositefull replacement, event/view agreement, duplicate preservation, survival acrossrenounceAdmin, the four empty cases (fuzzed for uncreated and malformed IDs), and the never-empty invariant.Cross-repo
base-std is the spec forerunner here, so this lands first and
base/basecatches up. The advisory fork-test job (continue-on-error: true) is expected to flagcompositePolicyChildIdsas missing until thebase/baseprecompile PR merges — the Rust implementation is written and green locally, and I'll link it here once it's open.Generated with Claude Code