diff --git a/test/lib/mocks/MockPolicyRegistry.sol b/test/lib/mocks/MockPolicyRegistry.sol index c95d0f1..a2b8c34 100644 --- a/test/lib/mocks/MockPolicyRegistry.sol +++ b/test/lib/mocks/MockPolicyRegistry.sol @@ -102,8 +102,8 @@ contract MockPolicyRegistry is IPolicyRegistry { /// @inheritdoc IPolicyRegistry function stageUpdateAdmin(uint64 policyId, address newAdmin) external { - uint256 packed = _requireCustom(policyId); - if (_decodeAdmin(packed) != msg.sender) revert Unauthorized(); + MockPolicyRegistryStorage.PolicyPacked memory packed = _requireCustom(policyId); + if (packed.admin != msg.sender) revert Unauthorized(); MockPolicyRegistryStorage.layout().pendingAdmins[policyId] = newAdmin; emit PolicyAdminStaged(policyId, msg.sender, newAdmin); } @@ -111,45 +111,46 @@ contract MockPolicyRegistry is IPolicyRegistry { /// @inheritdoc IPolicyRegistry function finalizeUpdateAdmin(uint64 policyId) external { MockPolicyRegistryStorage.Layout storage $ = MockPolicyRegistryStorage.layout(); - uint256 packed = $.policies[policyId]; - if (packed == 0) revert PolicyNotFound(); + MockPolicyRegistryStorage.PolicyPacked memory packed = $.policies[policyId]; + if (!MockPolicyRegistryStorage.existsSet(packed)) revert PolicyNotFound(); address pending = $.pendingAdmins[policyId]; if (pending == address(0)) revert NoPendingAdmin(); if (pending != msg.sender) revert Unauthorized(); - address previousAdmin = _decodeAdmin(packed); - $.policies[policyId] = _encode(msg.sender); + // Replace the admin lane in place. Solidity emits a single + // masked SSTORE; the existence byte at bits 248..255 is untouched. + $.policies[policyId].admin = msg.sender; delete $.pendingAdmins[policyId]; - emit PolicyAdminUpdated(policyId, previousAdmin, msg.sender); + emit PolicyAdminUpdated(policyId, packed.admin, msg.sender); } /// @inheritdoc IPolicyRegistry function renounceAdmin(uint64 policyId) external { MockPolicyRegistryStorage.Layout storage $ = MockPolicyRegistryStorage.layout(); - uint256 packed = $.policies[policyId]; - if (packed == 0) revert PolicyNotFound(); - if (_decodeAdmin(packed) != msg.sender) revert Unauthorized(); - // Admin lane cleared, exists flag (bit 160) survives so the - // policy stays observable via `policyExists` and the existence - // check on subsequent mutating calls still passes (with - // `Unauthorized` taking over as the rejection reason). - $.policies[policyId] = _encode(address(0)); + MockPolicyRegistryStorage.PolicyPacked memory packed = $.policies[policyId]; + if (!MockPolicyRegistryStorage.existsSet(packed)) revert PolicyNotFound(); + if (packed.admin != msg.sender) revert Unauthorized(); + // Admin lane cleared, existence byte survives so the policy + // stays observable via `policyExists` and the existence check + // on subsequent mutating calls still passes (with `Unauthorized` + // taking over as the rejection reason). + $.policies[policyId].admin = address(0); delete $.pendingAdmins[policyId]; emit PolicyAdminUpdated(policyId, msg.sender, address(0)); } /// @inheritdoc IPolicyRegistry function updateAllowlist(uint64 policyId, bool allowed, address[] calldata accounts) external { - uint256 packed = _requireCustom(policyId); + MockPolicyRegistryStorage.PolicyPacked memory packed = _requireCustom(policyId); if (_typeOf(policyId) != PolicyType.ALLOWLIST) revert IncompatiblePolicyType(); - if (_decodeAdmin(packed) != msg.sender) revert Unauthorized(); + if (packed.admin != msg.sender) revert Unauthorized(); _batchSetMembers({policyId: policyId, policyType: PolicyType.ALLOWLIST, value: allowed, accounts: accounts}); } /// @inheritdoc IPolicyRegistry function updateBlocklist(uint64 policyId, bool blocked, address[] calldata accounts) external { - uint256 packed = _requireCustom(policyId); + MockPolicyRegistryStorage.PolicyPacked memory packed = _requireCustom(policyId); if (_typeOf(policyId) != PolicyType.BLOCKLIST) revert IncompatiblePolicyType(); - if (_decodeAdmin(packed) != msg.sender) revert Unauthorized(); + if (packed.admin != msg.sender) revert Unauthorized(); _batchSetMembers({policyId: policyId, policyType: PolicyType.BLOCKLIST, value: blocked, accounts: accounts}); } @@ -181,7 +182,7 @@ contract MockPolicyRegistry is IPolicyRegistry { function policyExists(uint64 policyId) external view returns (bool) { if (policyId == ALWAYS_ALLOW_ID || policyId == ALWAYS_BLOCK_ID) return true; if (!_isWellFormed(policyId)) return false; - return MockPolicyRegistryStorage.layout().policies[policyId] != 0; + return MockPolicyRegistryStorage.existsSet(MockPolicyRegistryStorage.layout().policies[policyId]); } /// @inheritdoc IPolicyRegistry @@ -190,7 +191,7 @@ contract MockPolicyRegistry is IPolicyRegistry { // No fast path for built-in IDs needed: lazy init writes them with // a zero admin, so the normal storage read returns address(0) for // them just like for renounced policies and uncreated IDs. - return _decodeAdmin(MockPolicyRegistryStorage.layout().policies[policyId]); + return MockPolicyRegistryStorage.layout().policies[policyId].admin; } /// @inheritdoc IPolicyRegistry @@ -219,7 +220,7 @@ contract MockPolicyRegistry is IPolicyRegistry { $.nextCounter = counter + 1; } newPolicyId = _makeId({policyType: policyType, counter: counter}); - $.policies[newPolicyId] = _encode(admin); + $.policies[newPolicyId] = MockPolicyRegistryStorage.newPolicy(admin); emit PolicyCreated(newPolicyId, msg.sender, policyType); emit PolicyAdminUpdated(newPolicyId, address(0), admin); } @@ -227,7 +228,7 @@ contract MockPolicyRegistry is IPolicyRegistry { /// @dev Writes the two built-in policies into the `policies` mapping and /// advances `nextCounter` past them so custom policies start at /// `PolicyRegistryConstants.BUILTIN_POLICY_COUNT`. Both built-ins are - /// written with a renounced (zero) admin, so any later `require_admin` + /// written with a renounced (zero) admin, so any later admin-gated /// check against them rejects with `Unauthorized`. /// /// Idempotent: re-entry with `nextCounter >= BUILTIN_POLICY_COUNT` is @@ -238,9 +239,9 @@ contract MockPolicyRegistry is IPolicyRegistry { function _writeBuiltins() internal { MockPolicyRegistryStorage.Layout storage $ = MockPolicyRegistryStorage.layout(); if ($.nextCounter >= PolicyRegistryConstants.BUILTIN_POLICY_COUNT) return; - uint256 packed = _encode(address(0)); - $.policies[PolicyRegistryConstants.ALWAYS_ALLOW_ID] = packed; - $.policies[PolicyRegistryConstants.ALWAYS_BLOCK_ID] = packed; + MockPolicyRegistryStorage.PolicyPacked memory builtin = MockPolicyRegistryStorage.newPolicy(address(0)); + $.policies[PolicyRegistryConstants.ALWAYS_ALLOW_ID] = builtin; + $.policies[PolicyRegistryConstants.ALWAYS_BLOCK_ID] = builtin; $.nextCounter = PolicyRegistryConstants.BUILTIN_POLICY_COUNT; } @@ -259,25 +260,19 @@ contract MockPolicyRegistry is IPolicyRegistry { } } - function _requireCustom(uint64 policyId) internal view returns (uint256 packed) { + function _requireCustom(uint64 policyId) + internal + view + returns (MockPolicyRegistryStorage.PolicyPacked memory packed) + { packed = MockPolicyRegistryStorage.layout().policies[policyId]; - if (packed == 0) revert PolicyNotFound(); + if (!MockPolicyRegistryStorage.existsSet(packed)) revert PolicyNotFound(); } function _makeId(PolicyType policyType, uint56 counter) internal pure returns (uint64) { return (uint64(uint8(policyType)) << POLICY_ID_TYPE_SHIFT) | uint64(counter); } - /// @dev Composes a packed slot value. Always sets the exists bit; pass - /// `address(0)` to encode the post-renounce slot. - function _encode(address admin) internal pure returns (uint256) { - return (uint256(1) << MockPolicyRegistryStorage.EXISTS_BIT) | uint256(uint160(admin)); - } - - function _decodeAdmin(uint256 packed) internal pure returns (address) { - return address(uint160(packed)); - } - /// @dev Recovers the `PolicyType` from a well-formed `policyId`'s top byte. /// Caller MUST ensure `_isWellFormed(policyId)`; otherwise the cast panics. function _typeOf(uint64 policyId) internal pure returns (PolicyType) { diff --git a/test/lib/mocks/MockPolicyRegistryStorage.sol b/test/lib/mocks/MockPolicyRegistryStorage.sol index 7c260ee..dbc4955 100644 --- a/test/lib/mocks/MockPolicyRegistryStorage.sol +++ b/test/lib/mocks/MockPolicyRegistryStorage.sol @@ -19,17 +19,95 @@ pragma solidity ^0.8.20; /// `keccak256(abi.encode(uint256(keccak256("base.policy_registry")) - 1)) & ~bytes32(uint256(0xff))`. /// /// **Packed policy slot layout** (field `policies[id]`): -/// [255] exists flag (set on create, never cleared) -/// [254:160] unused +/// [255] exists flag (high bit of `existsByte`, never cleared) +/// [254:248] unused bits within `existsByte` (always zero) +/// [247:160] reserved (`reservedMiddle`, always zero) /// [159:0] admin address; zero after renounceAdmin -/// The exists bit survives renunciation, so `policies[id] == 0` +/// Solidity packs the `PolicyPacked` struct LSB-first into a single +/// 256-bit slot, so the struct field declaration order IS the binary +/// layout spec — the Rust impl mirrors the field order and types +/// (`Address` then `uint88` then `uint8`) with no comment-vs-code +/// drift surface. The bit-255 exists location is preserved as a +/// deliberate constraint (storage layout is frozen — see +/// `EXISTS_FLAG_BYTE` for the footgun this creates and the helpers +/// that hide it). +/// The exists bit survives renunciation, so an unset `existsByte` /// reliably means "never created". PolicyType is NOT stored — /// it is recovered from `policyId`'s top byte. +/// +/// **Polymorphism non-goal.** This struct holds the admin'd +/// ALLOWLIST/BLOCKLIST policy shape. Future composite policy +/// types (e.g. UNION/INTERSECT, immutable + multi-reference) +/// must NOT be overloaded onto this struct: Solidity's lack of +/// sum-types means doing so would force an awkward worst-of-both +/// layout. When composite types land, they get their own struct +/// and a parallel `mapping(uint64 => CompositePolicyPacked)`, +/// with the policy ID's top byte routing lookups between +/// mappings. library MockPolicyRegistryStorage { + /// @notice Packed storage word for an admin'd policy (ALLOWLIST or + /// BLOCKLIST). Solidity LSB-first packing: + /// bits 0..159 : admin + /// bits 160..247 : reservedMiddle (always zero) + /// bits 248..255 : existsByte — only bit 7 (slot bit 255) + /// carries the existence signal + /// @dev The `existsByte` field is a single byte but only its high + /// bit (= slot bit 255) is meaningful. This is a deliberate + /// consequence of preserving the bit-255 existence-flag + /// location while still expressing the slot as a Solidity + /// packed struct (Solidity has no arbitrary-width sub-byte + /// types, so a single-bit field at byte 31 has to ride along + /// in a `uint8`). **Direct writes to `existsByte` are + /// footgun-prone** — `existsByte = 1` lands the flag at slot + /// bit 248, NOT bit 255. Use `newPolicy()` to construct and + /// `existsSet()` to read; the helpers always write + /// `EXISTS_FLAG_BYTE` and read via inequality with zero. + /// + /// The `exists` signal is what lets the registry distinguish + /// "renounced" (admin zero, existsByte != 0) from "never + /// created" (admin zero, existsByte == 0). Without it, + /// BLOCKLIST policies with renounced admins would collide + /// with the zero-default. + struct PolicyPacked { + address admin; + uint88 reservedMiddle; + uint8 existsByte; + } + + /// @notice The byte value `existsByte` must hold to encode + /// "exists = true" while preserving the canonical bit-255 + /// existence-flag location. `0x80` sets bit 7 of the byte + /// at slot position 31, which equals slot bit 255. + /// @dev The `newPolicy` constructor always writes this value + /// when initializing a struct; reads check for non-zero via + /// `existsSet`. The `newPolicy` / `existsSet` helpers are + /// the only sanctioned API for the existence flag. + uint8 internal constant EXISTS_FLAG_BYTE = 0x80; + + /// @notice Constructs a new `PolicyPacked` with the existence flag + /// set in the canonical bit-255 position. + /// @dev Always use this constructor instead of struct-literal + /// initialization — direct `existsByte = ...` writes risk + /// landing the flag at the wrong bit. + function newPolicy(address admin) internal pure returns (PolicyPacked memory) { + return PolicyPacked({admin: admin, reservedMiddle: 0, existsByte: EXISTS_FLAG_BYTE}); + } + + /// @notice Whether `packed` has its existence flag set. + /// @dev Tolerant of any non-zero `existsByte` value (in case a + /// buggy writer lands a bit anywhere in the byte) — the + /// layout-pin tests catch wrong bit positions, so this + /// helper's job is just to be a correct-by-construction + /// existence check at runtime. + function existsSet(PolicyPacked memory packed) internal pure returns (bool) { + return packed.existsByte != 0; + } + /// @custom:storage-location erc7201:base.policy_registry struct Layout { - // Packed admin + exists flag; see header for layout. - mapping(uint64 policyId => uint256 packed) policies; + // Packed admin + exists flag via the `PolicyPacked` struct; see + // the struct definition and the header for the bit layout. + mapping(uint64 policyId => PolicyPacked packed) policies; // ALLOWLIST member: true → authorized. BLOCKLIST member: true → blocked. mapping(uint64 policyId => mapping(address account => bool)) members; // Staged pending admin for in-flight two-step admin transfers. @@ -100,7 +178,9 @@ library MockPolicyRegistryStorage { // outer key first to obtain an inner base slot, then hash the inner // key against that. - /// @notice Slot of `policies[policyId]` (the packed admin+exists uint256). + /// @notice Slot of `policies[policyId]` (the packed `PolicyPacked` + /// struct, a single 256-bit word — admin in bits 0..159, exists + /// flag at bit 255). function policySlot(uint64 policyId) internal pure returns (bytes32) { return keccak256(abi.encode(policyId, policiesBaseSlot())); } @@ -119,11 +199,21 @@ library MockPolicyRegistryStorage { // ============================================================ // PACKED-SLOT CODECS // ============================================================ - // See the library header for the `policies[id]` layout. + // Production code accesses `policies[id]` via the `PolicyPacked` + // struct's named fields — Solidity handles the bit math automatically. + // These pure codecs operate on a raw `uint256` (what `vm.load` returns + // for the slot) and exist for test-side use only: layout-pin tests + // that read the raw slot bytes can use them to extract fields without + // re-deriving the shifts at every callsite. + // + // The roundtrip tests in `MockPolicyRegistrySlotHelpers.t.sol` verify + // that these codecs' bit math matches Solidity's struct packing — so + // a codec drifting away from the canonical struct layout fails CI. - /// @notice Bit position of the existence flag (top bit). Leaves the - /// low 160 bits for the admin lane and reserves bits 161-254 - /// for future fields. + /// @notice Bit position of the `exists` flag within the packed slot. + /// Equals the high bit of `existsByte` (bit 7 of byte 31 = + /// slot bit 255). Preserved at this location as a deliberate + /// frozen-layout constraint. uint256 internal constant EXISTS_BIT = 255; /// @notice Extracts the policy admin (low 160 bits) from the packed slot. @@ -132,13 +222,15 @@ library MockPolicyRegistryStorage { return address(uint160(packed)); } - /// @notice Reads the existence flag. Lets tests distinguish "renounced" - /// (exists set, admin zero) from "never created" (both zero). + /// @notice Reads the exists flag. Lets tests distinguish "renounced" + /// (exists set, admin zero) from "never created" (slot zero). function policyExistsFromPacked(uint256 packed) internal pure returns (bool) { return (packed >> EXISTS_BIT) & 1 != 0; } /// @notice Composes a packed slot from an admin (exists bit always set). + /// Matches the binary layout Solidity emits for + /// `PolicyPacked({admin: admin, exists: true})`. function packPolicy(address admin) internal pure returns (uint256) { return (uint256(1) << EXISTS_BIT) | uint256(uint160(admin)); }