Address PR #26 review: pack policy IDs, canonical RoleGranted, dedup constants#33
Merged
Merged
Conversation
TRANSFER_SENDER, TRANSFER_RECEIVER, TRANSFER_EXECUTOR, MINT_RECEIVER now occupy one 256-bit slot (packedPolicyIds) instead of four mapping slots. Rare/variant policy types overflow to extraPolicyIds mapping. Cuts the transfer policy check to a single SLOAD reading both sender and receiver policy IDs. Mint/burn/executor paths similarly bench against the packed slot via inline shift+mask extraction. Addresses Conner's PR #26 review on hot-path policy lookups.
…okenCreated The Rust precompile atomically writes the initial admin role slot AND pushes the RoleGranted log in the same call frame. The mock now mirrors that: after vm.etch'ing the token and vm.store'ing name/symbol/cap, the factory calls token.grantRole(DEFAULT_ADMIN_ROLE, admin) during the privileged window so RoleGranted fires from the token's own context — no bootstrap surface added to MockB20 itself. Order: vm.etch -> vm.store metadata -> emit TokenCreated -> grantRole (canonical RoleGranted) -> initCalls -> set initialized. Drops admin field from TokenCreated since RoleGranted is now the canonical signal for the initial admin assignment. Addresses Conner's PR #26 review on bootstrap event canonicality.
…ants libraries Tests previously redeclared MINT_ROLE, BURN_ROLE, etc. inside B20Test. Now they import the canonical values from libraries co-located with the mocks, so MockB20 and tests share a single source of truth. Solidity contract types don't expose 'public constant' members at compile time (you can only call the runtime getter via an instance), so the constants live in libraries (B20Constants in MockB20.sol, PolicyRegistryConstants in MockPolicyRegistry.sol). The mock contracts re-expose them as 'public constant' delegating to the library values for the IB20 / IPolicyRegistry ABI. Sweep updates 30+ test files to reference B20Constants.MINT_ROLE, PolicyRegistryConstants.ALWAYS_ALLOW_ID, etc. Addresses Conner's PR #26 review on role constant duplication.
amiecorso
force-pushed
the
amie/policy-packing-and-canonical-events
branch
from
9da65e6 to
24f46fb
Compare
amiecorso
added a commit
that referenced
this pull request
May 20, 2026
Layered on top of the initial 26-mutation PoC, with mutations updated for the post-#35 (policy-storage refactor) impl. Adds 22 new curated mutations targeting the gap categories called out in the original PR discussion: address derivation, string encoding, factory validation, pause loop bounds, new policy lane wiring, permit cross-cutting (chainId / verifyingContract), stablecoin variant, order-sensitivity in balance updates. ** New mutations by group ** - Address derivation in _computeAddress (6): wrong variant byte shift, wrong decimals byte shift, wrong prefix byte, abi.encode vs encodePacked, isB20 wrong prefix, isB20 wrong shift. - String encoding in _writeString (3): short/long boundary off-by-one, long-string marker missing low bit, chunk count off-by-one. - Factory validation (3): decimals lower-bound off-by-one, upper-bound off-by-one, TokenAlreadyExists check inverted. - Pause bitmask (2): wrong shift direction in _isPaused, wrong bit in pause. - Pause loop bound (1): pausedFeatures count loop misses REDEEM. - Policy lane wiring (3): _readPolicyId returns wrong lane for each of TRANSFER_SENDER / TRANSFER_RECEIVER / TRANSFER_EXECUTOR. - Permit cross-cutting (2): DOMAIN_SEPARATOR hardcoded chainId (fork replay), hardcoded verifyingContract (cross-token replay). - Stablecoin variant (1): currency() returns base-token name from wrong namespace. - Order-sensitivity (1): _transfer balance signs reversed. - Refreshed 3 stale post-#35 patterns: TRANSFER_SENDER / TRANSFER_RECEIVER / MINT_RECEIVER policy-check skips now match the inline pattern (one SLOAD per op, manually inlined) rather than the old _checkPolicy helper. ** 4 real test gaps closed ** Initial 48-mutation run surfaced 4 survivors; all closed with new tests: 1. test_createToken_success_writesStringsAtEncodingBoundary -- exercises exactly 32-byte name (short/long boundary) AND 33-byte symbol (chunk-count boundary). Catches both _writeString boundary bugs. 2. test_getTokenAddress_pinsDownAbiEncoding -- recomputes the expected address using abi.encode externally and asserts the factory matches. Catches a switch to abi.encodePacked that internal-determinism tests wouldn't see (both predicted and actual paths share the same encoding choice). 3. test_pausedFeatures_success_includesRedeem -- explicit "pause REDEEM alone, assert it appears in pausedFeatures()". Catches a loop bound off-by-one that silently drops the highest-ordinal feature. ** Rebase fix ** test/unit/B20/roles/renounceRole.t.sol: my pre-rebase additions used bare DEFAULT_ADMIN_ROLE; main moved constants into B20Constants (per #33), so updated to B20Constants.DEFAULT_ADMIN_ROLE. forge test: 310 passed, 0 failed, 0 skipped. python script/mutate.py: 48 / 48 killed.
amiecorso
added a commit
that referenced
this pull request
May 20, 2026
) * Mutation testing: scripted harness, two real test-suite gaps closed ** Why this exists ** Coverage + passing tests prove tests execute the impl, NOT that the impl is constrained. We caught vacuous tests earlier in this project's life (the log-ordering test, the wrong mapping-slot derivation); mutation testing operationalizes "is the impl actually constrained" by introducing realistic bugs and checking that some test catches each. ** Tooling ** Tried vertigo-rs first: produces invalid Solidity 0.8.30 on our codebase (~107 mutations, all stillborn with compiler errors). Tried forge's purported native `--mutate` flag: doesn't exist in 1.5.1 despite some docs claiming it does. Built `script/mutate.py` instead: 26 hand-picked mutations representing realistic bug classes (inverted authorization checks, off-by-one boundaries, skipped policy / pause / role guards, swapped event arguments, wrong default values, permit replay vectors, allowance additive-vs-replace). ** Result ** 26/26 mutations killed by the test suite after this PR. Initial run surfaced 2 real gaps closed here: 1. test_renounceRole_success_adminCountStaysConsistent: verifies the internal adminCount tracker is correctly decremented on role revocation. Indirect check via "if the bookkeeping is right, the remaining sole admin's renounceRole should trip the last-admin guard." A mutation that flipped `-= 1` to `+= 1` on adminCount in _revokeRole was previously surviving silently -- token bookkeeping would drift over time without any test failing. 2. test_permit_success_secondPermitReplacesAllowance: verifies a second permit REPLACES the prior allowance value rather than ADDS to it. A single permit test can't catch the additive bug (0 + value == value); needs two permits with distinct values. This mutation was also previously surviving silently. ** Foundry.toml ** Added `ast = true` so build artifacts include AST data (required by mutation testing tools). Trivial size increase, no runtime cost. ** Going forward ** Re-run `python script/mutate.py` after any non-trivial MockB20 or MockTokenFactory change. The mutation set is hand-curated, not exhaustive, so add new entries as new bug classes come to mind. A surviving mutation = a test-suite gap. forge test: 302 passed, 0 failed, 0 skipped. * Expand mutation suite to 48 / close 4 more test gaps Layered on top of the initial 26-mutation PoC, with mutations updated for the post-#35 (policy-storage refactor) impl. Adds 22 new curated mutations targeting the gap categories called out in the original PR discussion: address derivation, string encoding, factory validation, pause loop bounds, new policy lane wiring, permit cross-cutting (chainId / verifyingContract), stablecoin variant, order-sensitivity in balance updates. ** New mutations by group ** - Address derivation in _computeAddress (6): wrong variant byte shift, wrong decimals byte shift, wrong prefix byte, abi.encode vs encodePacked, isB20 wrong prefix, isB20 wrong shift. - String encoding in _writeString (3): short/long boundary off-by-one, long-string marker missing low bit, chunk count off-by-one. - Factory validation (3): decimals lower-bound off-by-one, upper-bound off-by-one, TokenAlreadyExists check inverted. - Pause bitmask (2): wrong shift direction in _isPaused, wrong bit in pause. - Pause loop bound (1): pausedFeatures count loop misses REDEEM. - Policy lane wiring (3): _readPolicyId returns wrong lane for each of TRANSFER_SENDER / TRANSFER_RECEIVER / TRANSFER_EXECUTOR. - Permit cross-cutting (2): DOMAIN_SEPARATOR hardcoded chainId (fork replay), hardcoded verifyingContract (cross-token replay). - Stablecoin variant (1): currency() returns base-token name from wrong namespace. - Order-sensitivity (1): _transfer balance signs reversed. - Refreshed 3 stale post-#35 patterns: TRANSFER_SENDER / TRANSFER_RECEIVER / MINT_RECEIVER policy-check skips now match the inline pattern (one SLOAD per op, manually inlined) rather than the old _checkPolicy helper. ** 4 real test gaps closed ** Initial 48-mutation run surfaced 4 survivors; all closed with new tests: 1. test_createToken_success_writesStringsAtEncodingBoundary -- exercises exactly 32-byte name (short/long boundary) AND 33-byte symbol (chunk-count boundary). Catches both _writeString boundary bugs. 2. test_getTokenAddress_pinsDownAbiEncoding -- recomputes the expected address using abi.encode externally and asserts the factory matches. Catches a switch to abi.encodePacked that internal-determinism tests wouldn't see (both predicted and actual paths share the same encoding choice). 3. test_pausedFeatures_success_includesRedeem -- explicit "pause REDEEM alone, assert it appears in pausedFeatures()". Catches a loop bound off-by-one that silently drops the highest-ordinal feature. ** Rebase fix ** test/unit/B20/roles/renounceRole.t.sol: my pre-rebase additions used bare DEFAULT_ADMIN_ROLE; main moved constants into B20Constants (per #33), so updated to B20Constants.DEFAULT_ADMIN_ROLE. forge test: 310 passed, 0 failed, 0 skipped. python script/mutate.py: 48 / 48 killed.
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
1 participant
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.
Follow-up to #26 addressing Conner's review feedback. Three atomic commits, each tied to one comment.
1. Pack 4 hot-path policy IDs into single slot (c2cded2)
TRANSFER_SENDER,TRANSFER_RECEIVER,TRANSFER_EXECUTOR,MINT_RECEIVERnow occupy one 256-bit slot (packedPolicyIds) inMockB20Storageinstead of four separate mapping entries. Rare / variant / user-defined policy types overflow toextraPolicyIds._transferreads sender + receiver policy IDs in a single SLOAD via inline shift+mask extractiontransferFrom*adds the executor extract on top of the same slot_mint,burnBlockedsimilarly bench against the packed slot[63:0] sender | [127:64] receiver | [191:128] executor | [255:192] mintReceiver2. Emit canonical RoleGranted from token on bootstrap; drop
adminfromTokenCreated(c1f9143)The Rust precompile atomically writes the initial admin role slot AND pushes the
RoleGrantedlog in the same call frame. The mock now mirrors that: aftervm.etch'ing the token andvm.store'ing name/symbol/cap, the factory callstoken.grantRole(DEFAULT_ADMIN_ROLE, admin)during the privileged window soRoleGrantedfires from the token's own context — no bootstrap surface added to MockB20 itself.Order:
vm.etch→vm.storemetadata → emitTokenCreated→grantRole(canonicalRoleGranted) →initCalls→ setinitialized = true.Drops the
adminfield fromTokenCreatedsinceRoleGrantedis now the canonical signal for the initial admin assignment.3. Dedup role + policy constants into libraries (9da65e6)
Tests previously redeclared
MINT_ROLE,BURN_ROLE, etc. insideB20Test. Now they import the canonical values from libraries co-located with the mocks (B20ConstantsinMockB20.sol,PolicyRegistryConstantsinMockPolicyRegistry.sol).Solidity contract types don't expose
public constantmembers at compile time (you can only call the runtime getter via an instance), so the constants live in libraries; the mock contracts re-expose them aspublic constantdelegating to the library values for theIB20/IPolicyRegistryABI compliance.Sweep updates ~30 test files to reference
B20Constants.MINT_ROLE,PolicyRegistryConstants.ALWAYS_ALLOW_ID, etc.Test status
300 / 300 tests passing.