refactor(storage): structural slot-packing scaffolding + tighter layout pins#75
Merged
Conversation
Collaborator
|
I like this improvement a lot. Mind doing it for policy storage as well? I think that one may be more hairy given we do a single bit and it's the highest bit in base/base. Maybe we should change it to be next bit right after admin address? I'm also expecting the policy struct to be somewhat polymorphic in that we expect a future |
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
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
Addresses Conner's "comments-as-spec vs structural enforcement"
concern from Slack by both:
packed structs so the field declaration order IS the binary
layout spec (not a comment), and consumer code uses named field
access instead of inline shifts/masks.
classes of Rust-impl divergence (lane swaps, packing into
reserved bits, etc.) under fork mode.
Storage layout is frozen — the struct migration is bit-identical
to the prior hand-rolled
uint256packing, so the Rust side needszero changes. This is purely a Solidity-side clarity win plus
stronger CI cross-validation.
Commit 1:
test(storage): tighten packed-slot layout pins for Rust cross-validationThe full-layout tests under
test/unit/storage/are the primarycross-validation mechanism for catching Rust storage divergence in
fork mode (
LIVE_PRECOMPILES=true FOUNDRY_PROFILE=fork forge test ...).Three gaps closed:
redeemPolicyIdshad zero layout coverageNew
test/unit/storage/B20SecurityFullLayout.t.solmirrorsB20StablecoinFullLayout's pattern for the security variant'stwo added namespaces (
base.b20.security+base.b20.redeem),covering every field of both — including the
REDEEM_SENDER_POLICYfactory-seed default and the reserved upper 192 bits of
redeemPolicyIds.transferPolicyIdslanes weren't lane-swap-detectablePrevious version wrote the same
ALWAYS_BLOCK_IDinto all threelanes. A Rust impl that swapped any two lanes would pass for the
wrong reason. Fixed by creating four real custom policies in the
registry per run and using each as a distinct lane marker
(
updatePolicyvalidatespolicyExists, so synthetic markersaren't usable). Also: pause every defined
PausableFeature(notjust TRANSFER + MINT) so each bit position is pinned, and assertion
math is now raw bit ranges, not codec calls — codec helpers can
mask a buggy layout because they encode the same bit math.
PolicyRegistryFullLayoutdidn't pin reserved bits or post-renounce stateAdded explicit assertions that bits 160..254 of
policies[id]remain zero — both after
createPolicyAND afterrenounceAdmin— so a Rust impl can't sneak fields into the reserved range. The
post-renounce assertion also pins the exact bit pattern
policyExistsrelies on to distinguish "renounced" from "nevercreated" (admin cleared, exists flag survives, reserved still zero).
Commit 2:
refactor(storage): migrate packed policy slots to Solidity packed structsReplaces three
uint256packed slots with Solidity packed structs.Solidity packs struct fields LSB-first into a single 256-bit slot,
which is exactly the convention the Rust impl must reproduce —
so the struct field declaration order becomes the binary layout
spec with no comment-vs-code drift surface.
MockB20Storage.transferPolicyIdsuint256+ 4-lane commentstruct TransferPolicyIds { uint64 sender; uint64 receiver; uint64 executor; }MockB20Storage.mintPolicyIdsuint256+ 4-lane commentstruct MintPolicyIds { uint64 receiver; }MockB20RedeemStorage.redeemPolicyIdsuint256+ 4-lane commentstruct RedeemPolicyIds { uint64 sender; }Reserved lanes are now implicit (no field declared) so consumer
code can't accidentally write to them, and adding a new lane is a
natural Solidity edit that handles the bit math automatically.
Layout is bit-identical — verified by the layout-pin tests in
commit 1 still passing against the migrated storage (they read raw
slot bytes via
vm.load, so they exercise the Solidity-sidepacking rules without caring how the slot is declared).
Consumer code in
MockB20andMockB20Securitygoes fromto
\$.transferPolicyIds.receiver = receiverId;The five inline shifts in
MockB20._readPolicyId/_writePolicyIdplus the inline shifts in
_transfer,_mint,transferFrom,transferFromWithMemo,burnBlocked, and the redeem path inMockB20Security._redeemBurnall become named struct fieldaccesses.
Out of scope (intentional)
pausedVectorsstays as auint256bitmap — Solidity has nonative type for "256-bit bitmap of enum positions" and
struct { bool transfer; bool mint; ... }would break thelayout (bools get 8 bits each in Solidity).
MockPolicyRegistryStorage.policies[id]packed slot (existsat bit 255 with 95-bit gap) stays as-is. Conner flagged this
earlier as a low-bits-first convention violation; the cleanest
fix changes the binary layout (e.g. moving
existsto bit 160)which requires coordinated Rust storage migration — separate PR
conversation.
Tests
459 passed / 0 failed. Tests stay valid because the binary layout
under the struct migration is bit-identical to the hand-rolled
version. Both the per-mutator tests (which exercise the consumer
code) and the layout-pin tests (which read raw slot bytes) confirm
this.