From 614d48d11737f29203d484880dadc697bedd0caa Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 10 Jun 2026 11:36:47 -0700 Subject: [PATCH] test(policy-registry): cross-validate inactive-feature dispatch While a feature is inactive, the PolicyRegistry dispatcher routes view selectors to ABI dispatch but sends every other selector through the activation gate first, so error behavior depends on activation state: an unknown selector or malformed write returns FeatureNotActivated instead of being classified. Clients and monitoring can't then distinguish an inactive feature from bad calldata near activation boundaries. Add fork cross-validation for the dispatcher's inactive path: - run-fork-tests.sh: SKIP_ACTIVATE env var leaves named features (by name or 0x id, case-insensitive) un-activated so the inactive path is reachable; default keeps every feature active, so the existing run is unchanged. - test/unit/PolicyRegistry/dispatch_inactive.t.sol: fork-only tests that views stay callable while inactive, a malformed view reaches decode (not the gate), writes stay gated by FeatureNotActivated, and an unknown selector is classified rather than masked by the gate. The last encodes a not-yet-shipped dispatch fix, so it's gated behind POLICY_DISPATCH_FIX. - FORK_TESTING.md: document both env vars and the inactive-path invocation. Generated with Claude Code Co-Authored-By: Claude --- FORK_TESTING.md | 29 +++++ script/run-fork-tests.sh | 45 +++++++ .../PolicyRegistry/dispatch_inactive.t.sol | 122 ++++++++++++++++++ 3 files changed, 196 insertions(+) create mode 100644 test/unit/PolicyRegistry/dispatch_inactive.t.sol diff --git a/FORK_TESTING.md b/FORK_TESTING.md index 45c8f54..bd022c4 100644 --- a/FORK_TESTING.md +++ b/FORK_TESTING.md @@ -97,6 +97,35 @@ Forward any `forge test` flag through the script: ./script/run-fork-tests.sh -vvvv --match-test test_transfer_success_debitsSender ``` +## Exercising the inactive-feature dispatch path + +By default the script activates every gated feature before `forge test`, so the +suite never sees a feature in its inactive state. To cross-validate the +dispatcher's *inactive* behavior (error semantics must not depend on whether a +feature is active), set `SKIP_ACTIVATE` to a comma-separated list of feature +names (or raw `0x` ids) to leave un-activated: + +```bash +SKIP_ACTIVATE=POLICY_REGISTRY ./script/run-fork-tests.sh \ + --match-contract PolicyRegistryDispatchInactive +``` + +The inactive-dispatch tests (`test/unit/PolicyRegistry/dispatch_inactive.t.sol`) +also normalize the feature to inactive themselves, so they're correct under the +default run too; `SKIP_ACTIVATE` additionally validates the never-activated +baseline. One assertion (that an unknown selector is classified *before* the +activation gate rather than masked by `FeatureNotActivated`) encodes a fix that +isn't in the Rust impl yet, so it's gated behind `POLICY_DISPATCH_FIX`: + +```bash +SKIP_ACTIVATE=POLICY_REGISTRY POLICY_DISPATCH_FIX=true ./script/run-fork-tests.sh \ + --match-contract PolicyRegistryDispatchInactive +``` + +Run it that way against a build that carries the dispatch-ordering fix (e.g. via +the [patch] local-clone override below) to enforce the regression; leave it +unset against stock builds so the default run stays green. + ## When the precompiles update — the loop This is the main workflow. base/base's precompile crate changes; you want diff --git a/script/run-fork-tests.sh b/script/run-fork-tests.sh index a2c2dc4..0ab7c08 100755 --- a/script/run-fork-tests.sh +++ b/script/run-fork-tests.sh @@ -28,6 +28,12 @@ # (default: 0x9965507D1a55bcC2695C58ba16FB37d819B0A4dc, the # canonical local-dev admin) # ANVIL_LOG anvil stdout/stderr log path (default: /tmp/anvil.log) +# SKIP_ACTIVATE comma-separated feature names or 0x ids to leave +# un-activated (default: none, so every feature is activated). +# Use to exercise the inactive-feature dispatch path, e.g. +# SKIP_ACTIVATE=POLICY_REGISTRY to run the policy-registry +# inactive-dispatch regression tests. Names and ids are +# matched case-insensitively. # # Exit codes: # 0 forge test exit 0 (all targeted tests pass) @@ -84,11 +90,45 @@ FEATURE_IDS=( 0xecfa0def2c10020caaf65e6155aa69c84b24892aaef76eeac52e0e2b3a0b8601 # B20_STABLECOIN ) +# Optional set of features to leave UN-activated (see header). Matched against +# both the canonical name and the raw id, case-insensitively. Default empty, so +# the standard cross-validation run activates everything as before. +SKIP_ACTIVATE="${SKIP_ACTIVATE:-}" + # ── Helpers ─────────────────────────────────────────────────────────────────── log() { echo "[run-fork-tests] $*" >&2; } die() { echo "[run-fork-tests] ERROR: $*" >&2; exit 2; } +# Canonical name for a feature id (mirrors the comments on FEATURE_IDS and the +# Solidity ActivationRegistryFeatureList). Empty string for an unknown id. +feature_name() { + case "$1" in + 0xcdcc772fe4cbdb1029f822861176d09e646db96723d4c1e82ddfdeb8163ef54c) echo B20_ASSET ;; + 0xb582ebae03f16fee49a6763f78df482fb11ae73f103ed0d330bbe556aa90a43f) echo POLICY_REGISTRY ;; + 0xecfa0def2c10020caaf65e6155aa69c84b24892aaef76eeac52e0e2b3a0b8601) echo B20_STABLECOIN ;; + *) echo "" ;; + esac +} + +# Returns 0 (skip) if feature id $1 is named in SKIP_ACTIVATE, by either its +# canonical name or its raw id. Case-insensitive; whitespace-tolerant. +should_skip_activate() { + [[ -z "$SKIP_ACTIVATE" ]] && return 1 + local fid="$1" name id_uc entry + name="$(feature_name "$fid")" + id_uc="$(printf '%s' "$fid" | tr '[:lower:]' '[:upper:]')" + local IFS=',' + for entry in $SKIP_ACTIVATE; do + entry="$(printf '%s' "$entry" | tr -d '[:space:]' | tr '[:lower:]' '[:upper:]')" + [[ -z "$entry" ]] && continue + if [[ "$entry" == "$name" || "$entry" == "$id_uc" ]]; then + return 0 + fi + done + return 1 +} + rpc() { local method="$1"; shift local params="$1"; shift @@ -111,6 +151,7 @@ log "forge: $FORGE_BIN" log "port: $PORT" log "activation admin: $ACTIVATION_ADMIN" log "log file: $LOG_FILE" +log "skip-activate: ${SKIP_ACTIVATE:-}" # ── Launch anvil ────────────────────────────────────────────────────────────── @@ -144,6 +185,10 @@ rpc anvil_setBalance "[\"$ACTIVATION_ADMIN\", \"0xffffffffffffffff\"]" > /dev/nu rpc anvil_impersonateAccount "[\"$ACTIVATION_ADMIN\"]" > /dev/null for fid in "${FEATURE_IDS[@]}"; do + if should_skip_activate "$fid"; then + log "leaving feature un-activated: $(feature_name "$fid") $fid [SKIP_ACTIVATE]" + continue + fi log "activating feature $fid" out=$(cast send --rpc-url "http://localhost:$PORT" --from "$ACTIVATION_ADMIN" \ --unlocked "$REGISTRY" "activate(bytes32)" "$fid" 2>&1) || \ diff --git a/test/unit/PolicyRegistry/dispatch_inactive.t.sol b/test/unit/PolicyRegistry/dispatch_inactive.t.sol new file mode 100644 index 0000000..f28d4e6 --- /dev/null +++ b/test/unit/PolicyRegistry/dispatch_inactive.t.sol @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +import {IActivationRegistry} from "base-std/interfaces/IActivationRegistry.sol"; +import {IPolicyRegistry} from "base-std/interfaces/IPolicyRegistry.sol"; +import {StdPrecompiles} from "base-std/StdPrecompiles.sol"; + +import {ActivationRegistryFeatureList} from "base-std-test/lib/mocks/ActivationRegistryFeatureList.sol"; +import {PolicyRegistryTest} from "base-std-test/lib/PolicyRegistryTest.sol"; + +/// @title PolicyRegistry dispatch tests for the inactive-feature path. +/// +/// @notice PolicyRegistry error semantics must not depend on activation state: while inactive, +/// views stay callable and reach decode, writes stay gated, and an unknown selector is +/// classified rather than masked by `FeatureNotActivated`. +/// +/// @dev Fork-only: `MockPolicyRegistry` models no activation gate, so this path exists only on the +/// live precompile. Each test skips unless `LIVE_PRECOMPILES` is set and normalizes the feature +/// to inactive first. Raw calls are used where the case needs malformed/unknown calldata. +contract PolicyRegistryDispatchInactiveTest is PolicyRegistryTest { + bytes32 internal constant FEATURE = ActivationRegistryFeatureList.POLICY_REGISTRY; + + function _forkMode() internal view returns (bool) { + return vm.envOr("LIVE_PRECOMPILES", false); + } + + /// @dev Deactivate the feature if active; no-op otherwise (idempotent across both run modes). + function _ensureInactive() internal { + if (StdPrecompiles.ACTIVATION_REGISTRY.isActivated(FEATURE)) { + vm.prank(StdPrecompiles.ACTIVATION_REGISTRY.admin()); + StdPrecompiles.ACTIVATION_REGISTRY.deactivate(FEATURE); + } + } + + /// @dev Whether `data` is exactly a `FeatureNotActivated(FEATURE)` revert payload. + function _isFeatureNotActivated(bytes memory data) internal pure returns (bool) { + return + keccak256(data) + == keccak256(abi.encodeWithSelector(IActivationRegistry.FeatureNotActivated.selector, FEATURE)); + } + + function _viewSelectors() internal pure returns (bytes4[4] memory) { + return [ + IPolicyRegistry.isAuthorized.selector, + IPolicyRegistry.policyExists.selector, + IPolicyRegistry.policyAdmin.selector, + IPolicyRegistry.pendingPolicyAdmin.selector + ]; + } + + function _writeSelectors() internal pure returns (bytes4[7] memory) { + return [ + IPolicyRegistry.createPolicy.selector, + IPolicyRegistry.createPolicyWithAccounts.selector, + IPolicyRegistry.stageUpdateAdmin.selector, + IPolicyRegistry.finalizeUpdateAdmin.selector, + IPolicyRegistry.renounceAdmin.selector, + IPolicyRegistry.updateAllowlist.selector, + IPolicyRegistry.updateBlocklist.selector + ]; + } + + function _isKnownSelector(bytes4 selector) internal pure returns (bool) { + bytes4[4] memory views = _viewSelectors(); + for (uint256 i = 0; i < views.length; i++) { + if (selector == views[i]) return true; + } + bytes4[7] memory writes = _writeSelectors(); + for (uint256 i = 0; i < writes.length; i++) { + if (selector == writes[i]) return true; + } + return false; + } + + /// @notice Views stay callable while inactive. + function test_dispatch_success_viewsCallableWhileInactive(uint64 policyId, address account) public { + vm.skip(!_forkMode()); + _ensureInactive(); + + policyRegistry.isAuthorized(policyId, account); + policyRegistry.policyExists(policyId); + policyRegistry.policyAdmin(policyId); + policyRegistry.pendingPolicyAdmin(policyId); + } + + /// @notice An unknown selector is classified, not masked by the activation gate. + /// @dev Gated behind `POLICY_DISPATCH_FIX`: stock builds still return `FeatureNotActivated` here + /// until the dispatch-ordering fix is in the pinned impl. + function test_dispatch_revert_unknownSelectorNotMaskedByActivation(bytes4 selector) public { + vm.skip(!(_forkMode() && vm.envOr("POLICY_DISPATCH_FIX", false))); + vm.assume(!_isKnownSelector(selector)); + _ensureInactive(); + + (bool ok, bytes memory ret) = StdPrecompiles.POLICY_REGISTRY_ADDRESS.staticcall(abi.encodePacked(selector)); + assertFalse(ok, "unknown selector must revert"); + assertFalse(_isFeatureNotActivated(ret), "unknown selector must not be masked while inactive"); + } + + /// @notice A malformed view reaches ABI decode rather than the gate. + /// @dev Bare selector (no args) is too short to decode for every view. + function test_dispatch_revert_malformedViewReachesDecode(uint8 viewIdx) public { + vm.skip(!_forkMode()); + _ensureInactive(); + + bytes4 selector = _viewSelectors()[viewIdx % 4]; + (bool ok, bytes memory ret) = StdPrecompiles.POLICY_REGISTRY_ADDRESS.staticcall(abi.encodePacked(selector)); + assertFalse(ok, "malformed view call must revert"); + assertFalse(_isFeatureNotActivated(ret), "malformed view must reach decode, not the gate"); + } + + /// @notice Write selectors stay gated by `FeatureNotActivated` while inactive. + /// @dev Gate fires before arg decode, so a bare (malformed) write selector still hits it. + function test_dispatch_revert_writeGatedWhileInactive(uint8 writeIdx) public { + vm.skip(!_forkMode()); + _ensureInactive(); + + bytes4 selector = _writeSelectors()[writeIdx % 7]; + (bool ok, bytes memory ret) = StdPrecompiles.POLICY_REGISTRY_ADDRESS.call(abi.encodePacked(selector)); + assertFalse(ok, "write call must revert while inactive"); + assertTrue(_isFeatureNotActivated(ret), "malformed write must stay gated while inactive"); + } +}