From 93a33d4e83529a12aecf13312e1890e4b0f8c94a Mon Sep 17 00:00:00 2001 From: David Meister Date: Fri, 18 Sep 2026 15:28:03 +0000 Subject: [PATCH 1/3] docs: comments state current behaviour, part 1 Co-Authored-By: Claude Opus 5 (1M context) --- src/lib/LibMemoryKV.sol | 20 ++-- test/lib/LibMemoryKVHandle.t.sol | 4 +- test/lib/LibMemoryKVSlow.t.sol | 4 +- test/src/lib/LibMemoryKV.array.t.sol | 13 +-- test/src/lib/LibMemoryKV.bisect.t.sol | 104 +++++++------------ test/src/lib/LibMemoryKV.crossFunction.t.sol | 18 ++-- test/src/lib/LibMemoryKV.errorAbi.t.sol | 10 +- test/src/lib/LibMemoryKV.exportAlloc.t.sol | 42 +++----- test/src/lib/LibMemoryKV.exportWalk.t.sol | 11 +- test/src/lib/LibMemoryKV.frameLocal.t.sol | 4 +- test/src/lib/LibMemoryKV.gasClaims.t.sol | 13 +-- test/src/lib/LibMemoryKV.getMatch.t.sol | 9 +- test/src/lib/LibMemoryKV.getScratch.t.sol | 6 +- test/src/lib/LibMemoryKV.getWalk.t.sol | 16 ++- test/src/lib/LibMemoryKV.saturate.t.sol | 3 +- test/src/lib/LibMemoryKV.setCapacity.t.sol | 13 +-- 16 files changed, 105 insertions(+), 185 deletions(-) diff --git a/src/lib/LibMemoryKV.sol b/src/lib/LibMemoryKV.sol index b803eaa..46aadd4 100644 --- a/src/lib/LibMemoryKV.sol +++ b/src/lib/LibMemoryKV.sol @@ -62,9 +62,7 @@ library LibMemoryKV { uint256 internal constant NODE_BYTES = 0x60; /// Thrown when an insert would allocate its node at a pointer above - /// `POINTER_MASK`, the widest head pointer a list slot can hold, so the - /// slot would truncate it and the bits above the slot would overwrite the - /// neighbouring slots and the word count. + /// `POINTER_MASK`, the widest head pointer a list slot can hold. /// /// Only the head is bounded: the rest of the node MAY extend above /// `POINTER_MASK`, as every field is reached by full width arithmetic from @@ -72,11 +70,8 @@ library LibMemoryKV { /// @param pointer The offending pointer, not the bound it crossed. error MemoryKVOverflow(uint256 pointer); - /// Thrown when an insert would push the word count past `POINTER_MASK`. - /// The count is written by shifting into the top bits of `MemoryKV`, so - /// without this the sum silently loses its high bit, and `toBytes32Array`, - /// which sizes its allocation from the count, then copies every pair it - /// walks past the end of that array. + /// Thrown when an insert would push the word count past `POINTER_MASK`, + /// the widest word count its slot can hold. /// @param length The word count the insert would have produced, not the /// stored count before it and not the `POINTER_MASK` bound it crossed. error MemoryKVLengthOverflow(uint256 length); @@ -236,8 +231,8 @@ library LibMemoryKV { /// @return array All the keys and values copied pairwise into a `bytes32[]`. /// The pair order is unspecified and MUST NOT be relied upon; a caller that /// needs a canonical form MUST sort. - // Slither is not wrong about the cyclomatic complexity but I don't know - // another way to implement the bisect and keep the gas savings. + // The cyclomatic complexity slither counts is the bisect's branches, one + // per node of the tree over the head pointers. //slither-disable-next-line cyclomatic-complexity function toBytes32Array(MemoryKV kv) internal pure returns (bytes32[] memory array) { uint256 mask16 = type(uint16).max; @@ -268,9 +263,8 @@ library LibMemoryKV { // Bisect. The gas this tree saves over a linear loop is documented // in the NatSpec above. - // The internal scoping blocks are to provide some safety against - // typos causing the incorrect symbol to be referenced by enforcing - // each symbol is as tightly scoped as it can be. + // Each symbol is declared in the smallest block that holds every + // use of it, so a use outside that block does not compile. let cursor := add(array, 0x20) { // Remove the length from kv before iffing to save ~100 gas. diff --git a/test/lib/LibMemoryKVHandle.t.sol b/test/lib/LibMemoryKVHandle.t.sol index 2eb00f9..544bd63 100644 --- a/test/lib/LibMemoryKVHandle.t.sol +++ b/test/lib/LibMemoryKVHandle.t.sol @@ -36,8 +36,8 @@ contract LibMemoryKVHandleTest is Test { /// A node crafted where an insert into an empty store would put its node, /// under a handle naming it with one pair's count, is exactly what that /// insert builds: the same handle, the same three words and the same - /// allocation. The memory under each is dirtied first, so a word either - /// one leaves unwritten reads as the sentinel. + /// allocation. The memory under each is dirtied with the same sentinel + /// first. function testCraftNodeAndHandleWithBuildWhatAnInsertBuilds(MemoryKVKey key, MemoryKVVal value) external pure { bytes32 sentinel = keccak256(abi.encode(key, value)); Pointer at = LibPointer.allocatedMemoryPointer(); diff --git a/test/lib/LibMemoryKVSlow.t.sol b/test/lib/LibMemoryKVSlow.t.sol index bb04394..03f269a 100644 --- a/test/lib/LibMemoryKVSlow.t.sol +++ b/test/lib/LibMemoryKVSlow.t.sol @@ -11,9 +11,7 @@ import {LibMemoryKVSlow} from "test/lib/LibMemoryKVSlow.sol"; contract LibMemoryKVSlowTest is Test { /// The linear export takes the array from the free pointer and leaves the - /// free pointer past every word it writes into it. An allocation short of - /// what the copy writes leaves exported pairs sitting in memory that the - /// next allocation hands out, which nothing reading only the array can see. + /// free pointer past every word it writes into it. function testSlowLinearExportAllocatesWhatItWrites(bytes32[] memory kvs) external pure { vm.assume(kvs.length % 2 == 0); diff --git a/test/src/lib/LibMemoryKV.array.t.sol b/test/src/lib/LibMemoryKV.array.t.sol index 395f190..2bb94b3 100644 --- a/test/src/lib/LibMemoryKV.array.t.sol +++ b/test/src/lib/LibMemoryKV.array.t.sol @@ -134,10 +134,8 @@ contract LibMemoryKVArrayTest is Test { assertEq(slowExists, true); assertEq(roundExists, true); assertEq(slowVal, roundVal); - // A lookup on both sides is the same reader twice: one that answers - // with the wrong word of a pair answers with it on both sides and - // the comparison holds whatever was exported. The value the model - // holds is therefore read out of its array by index as well. + // The exported value is also the word the model holds after the + // key, read out of its array by index. assertEq(roundVal, slowKVs[i + 1]); } } @@ -156,11 +154,8 @@ contract LibMemoryKVArrayTest is Test { assertEq(array.length, arrayLinear.length); - // Counted over every pair at once, a pair exported twice pays for a - // pair not exported at all, so each pair is counted on its own. The - // linear walk names the pairs because it visits each of the 15 slots - // exactly once, so a list the bisect reads twice shows up as a linear - // pair matched twice. + // The linear walk visits each list exactly once, and every pair it + // exports is counted on its own in the bisect's export: exactly once. for (uint256 i = 0; i < arrayLinear.length; i += 2) { assertEq(countPair(array, arrayLinear[i], arrayLinear[i + 1]), 1, "each pair exported exactly once"); } diff --git a/test/src/lib/LibMemoryKV.bisect.t.sol b/test/src/lib/LibMemoryKV.bisect.t.sol index 14fe16f..c21b39c 100644 --- a/test/src/lib/LibMemoryKV.bisect.t.sol +++ b/test/src/lib/LibMemoryKV.bisect.t.sol @@ -24,32 +24,23 @@ import {setFreePointer, raiseFreePointerTo} from "test/lib/LibFreeMemory.sol"; /// lane 7 is structurally zero. The tree then collapses the node that would /// have covered that zero lane — slot 14 is reached by `shr(0x20, p00)` with no /// `and(mask16, ...)` scrub, where the mirroring leaf of the low half has one. -/// That omission is sound only while the root split has already zeroed the -/// length, a coupling between two lines twelve apart, so the split and both -/// halves are pinned in one file rather than split across files that cannot see -/// each other's assumptions. -/// -/// Two layers, and both are wanted. +/// That leaf reads slot 14 alone because the root split has already zeroed the +/// length. /// /// The named tests each occupy one known slot, or one known pair, or one known -/// boundary. A store that populates many slots at once fails as "some pair is -/// missing" with no indication of which; a store built from one named slot -/// fails as that slot's key and value, and the test name says which slot. +/// boundary, and the test name says which. /// -/// The enumerations then run every combination of which slots are occupied. -/// That is the tree's whole input: each of its twenty eight guards tests a -/// window of `kv` against zero, and nothing it does branches on what a key or a -/// value is. Fifteen slots is 32768 combinations, so the enumerations cover the -/// named cases too, and nothing is sampled. The named cases are kept anyway, -/// for the name on the failure. +/// The enumerations run every combination of which slots are occupied. That is +/// the tree's whole input: each of its twenty eight guards tests a window of +/// `kv` against zero, and nothing it does branches on what a key or a value is. +/// Fifteen slots is 32768 combinations, so the enumerations cover the named +/// cases too, and nothing is sampled. /// -/// `slotKeys` is what makes either affordable: one constant key per slot, so an -/// occupancy is chosen rather than searched for, and no test in this file takes -/// a fuzz input. +/// `slotKeys` is one constant key per slot, so every test here chooses its +/// occupancy and none takes a fuzz input. /// /// Every node of every case an enumeration builds is allocated from a free -/// memory pointer that is rewound between cases. Unrewound, the 32768 cases -/// walk that pointer past the `0xFFFF` ceiling `set` enforces after 115 of them. +/// memory pointer that is rewound between cases. contract LibMemoryKVBisectTest is Test { using LibMemoryKV for MemoryKV; @@ -73,10 +64,9 @@ contract LibMemoryKVBisectTest is Test { /// Stands for "no mask", which a 15 bit mask cannot collide with. uint256 internal constant NO_MASK = type(uint256).max; - /// Values sit above every key, so a copy that reads the key where the value - /// belongs, or that reads another slot's value, is a different word. A - /// value is this base plus the slot it belongs to, which is how - /// `exportMatches` reads a pair's slot back out of it. + /// Values sit above every key, so no value word equals a key word. A value + /// is this base plus the slot it belongs to, so no two slots share a value, + /// which is how `exportMatches` reads a pair's slot back out of it. uint256 internal constant VALUE_BASE = 0x100; /// One key per slot: the smallest positive integer whose 32 byte big endian @@ -172,10 +162,8 @@ contract LibMemoryKVBisectTest is Test { } /// Exports the store and holds it to the pairs `mask` names, one pair per - /// named slot and each exported exactly once. A window that is too wide - /// duplicates a pair, one that is too narrow drops it, and a leaf that does - /// not carry the cursor forward is overwritten by the next one — all three - /// are a count other than one. Order is not checked, per `exportMatches`. + /// named slot and each exported exactly once. Order is not checked, per + /// `exportMatches`. function checkExportedPairs(MemoryKV kv, uint256 mask, bytes32[] memory keys) internal pure { bytes32[] memory array = kv.toBytes32Array(); assertEq(array.length, occupiedCount(mask) * 2, "one pair per occupied slot"); @@ -213,10 +201,8 @@ contract LibMemoryKVBisectTest is Test { checkExportedPairs(kv, mask, keys); } - /// A single key in `slot` and nothing else. The exported array must be - /// exactly that one pair, so a bisect that never visits `slot`, or that - /// reaches it with a mangled pointer, exports something other than the key - /// where the key belongs. + /// A single key in `slot` and nothing else. The exported array is exactly + /// that one pair: the slot's key, then its value. function checkSoleSlot(uint256 slot) internal pure { bytes32[] memory keys = slotKeys(); MemoryKV kv = storeForOccupancy(2 ** slot, keys); @@ -285,16 +271,13 @@ contract LibMemoryKVBisectTest is Test { } /// Slot 14 is the leaf the collapsed path reaches. Alone in the store its - /// 16 bits are the only nonzero bits of `p00`, so a shift of the wrong - /// distance yields a pointer into unwritten memory rather than the node. + /// 16 bits are the only nonzero bits of `p00`, and the export reaches its + /// node through them. function testHighSlot14SoleExport() public pure { checkSoleSlot(14); } - /// Two known slots and nothing else. A mask or shift that folds one onto - /// the other, or a window that covers one twice, changes how often a pair - /// appears rather than merely dropping it — which neither slot alone would - /// reveal. + /// Two known slots and nothing else, each exported exactly once. function checkSlotPair(uint256 slotA, uint256 slotB) internal pure { checkNamedOccupancy(2 ** slotA | 2 ** slotB); } @@ -326,26 +309,23 @@ contract LibMemoryKVBisectTest is Test { } /// Slots 12 and 13 are the ordinary leaves under the same node as slot 14, - /// occupied while slot 14 is not. The collapsed path must read as empty - /// here: anything left in the bits above slot 14 turns into a pointer and - /// costs the store a pair. + /// occupied while slot 14 is not. The collapsed path reads as empty here, + /// and the export is exactly the two pairs. function testHighSlots1213Siblings() public pure { checkSlotPair(12, 13); } /// The two halves of the root split meet between slot 7 and slot 8. Slots 7 /// and 8 are not siblings — they are the outermost leaves of different - /// halves — so one key each side of that boundary is the discriminator for - /// the split itself: a window that overlaps exports one of them twice, and - /// a window that leaves a gap drops one. + /// halves — so this is one key each side of the split, each exported + /// exactly once. function testRootSplitBoundarySlots7And8() public pure { checkSlotPair(7, 8); } /// All three leaves below the node that holds slot 14, so the collapsed - /// path and the ordinary path run against each other. A mask that lets the - /// collapsed leaf through to the ordinary subtree, or the reverse, changes - /// how often a pair appears rather than merely dropping it. + /// path and the ordinary path run in one export, and each of the three + /// pairs is exported exactly once. function testHighSlot14WithBothOrdinaryLeaves() public pure { checkNamedOccupancy(2 ** 12 | 2 ** 13 | 2 ** 14); } @@ -377,8 +357,7 @@ contract LibMemoryKVBisectTest is Test { /// Whether `array` holds exactly the pairs `mask` must export, as a /// multiset. `toBytes32Array` documents its pair order as unspecified, so - /// position is deliberately not checked: two blocks of the tree swapped - /// emit the same pairs in a different order and are not a defect. + /// position is not checked. /// /// Each value is `VALUE_BASE + slot`, so the slot a pair claims is read out /// of its own value, and the pair is then held to the key that slot must @@ -386,10 +365,6 @@ contract LibMemoryKVBisectTest is Test { /// multiset equality without a scan per pair: a slot exported twice is /// caught by its bit already being set, and one dropped, duplicated or /// invented by the bitsets differing. - /// - /// The named tests above check the same thing through `countPair`, which - /// costs a scan per pair but names the slot it checked. Here the whole - /// point is 32768 cases, so the check is the cheap one. function exportMatches(bytes32[] memory array, uint256 mask, bytes32[] memory keys) internal pure returns (bool) { if (array.length % 2 != 0) { return false; @@ -449,15 +424,13 @@ contract LibMemoryKVBisectTest is Test { assertEq(failed, NO_MASK, "occupancy mask whose export did not match"); } - /// The fifteen key constants are the whole reason a combination can be - /// chosen rather than searched for, and every test below reads its result - /// through the slot each key claims. Should the hash that `get` and `set` - /// share ever move, the keys stop naming the slots they claim and the tests - /// silently cover something other than what they report. + /// Each key constant hashes into the slot it is claimed for, and every + /// chain key into slot 0. Every test in this file reads its result through + /// the slot each key claims. /// - /// Occupying all fifteen at once is what makes them fifteen distinct slots - /// rather than fifteen keys that each hash somewhere. `set` routes on the - /// key alone, so that plus one key per slot is every occupancy. + /// Occupying all fifteen at once populates fifteen distinct slots. `set` + /// routes on the key alone, so that plus one key per slot is every + /// occupancy. function testKeyConstantsLandWhereClaimed() public pure { bytes32[] memory keys = slotKeys(); for (uint256 slot = 0; slot < LibMemoryKV.LIST_COUNT; slot++) { @@ -477,17 +450,16 @@ contract LibMemoryKVBisectTest is Test { } /// Every occupancy combination again with every pointer at or above - /// `POINTER_HIGH_BIT`, so both states of a pointer's top bit are enumerated - /// rather than left to wherever the allocator happened to be. + /// `POINTER_HIGH_BIT`, so both states of a pointer's top bit are + /// enumerated. function testEveryOccupancyCombinationExportedFromHighPointers() public pure { raiseFreePointerTo(POINTER_HIGH_BIT); checkEveryOccupancy(POINTER_HIGH_BIT, LibMemoryKV.POINTER_MASK); } /// The length is not a slot. It sits directly above slot 14, which is the - /// leaf the collapsed path reaches without scrubbing, so a root split that - /// carried the length into the tree would read it as that leaf's pointer. - /// Every pair here is on one low list, which leaves the whole high half + /// leaf the collapsed path reaches without scrubbing, and the export reads + /// no pointer out of it. Every pair here is on one low list, which leaves the whole high half /// empty while the length is far larger than any single insert makes it. /// /// It is also the only list built longer than one node, so it is the only diff --git a/test/src/lib/LibMemoryKV.crossFunction.t.sol b/test/src/lib/LibMemoryKV.crossFunction.t.sol index 50ecbb6..23b9045 100644 --- a/test/src/lib/LibMemoryKV.crossFunction.t.sol +++ b/test/src/lib/LibMemoryKV.crossFunction.t.sol @@ -10,10 +10,12 @@ import {lengthOf} from "test/lib/LibMemoryKVHandle.sol"; import {assertValue} from "test/lib/LibMemoryKVAssert.sol"; /// @title LibMemoryKVCrossFunctionTest -/// Two claims about a `MemoryKV` handle that no mutation of the library can -/// falsify, because they are about the handle as a `uint256` rather than about -/// anything the library computes: every bit of it survives an external call, -/// and the update/insert asymmetry holds however many handles are live at once. +/// Two properties of `MemoryKV` handles that take more than one call frame or +/// more than one live handle to observe. The packed word `set` builds, its +/// count at `COUNT_BIT_OFFSET` and list `i`'s head at `i * SLOT_BITS` over +/// nodes `NODE_BYTES` apart, survives an external call bit for bit. An update +/// reaches every live handle holding the key, while an insert changes only the +/// handle `set` returns. contract LibMemoryKVCrossFunctionTest is Test { using LibMemoryKV for MemoryKV; @@ -41,11 +43,9 @@ contract LibMemoryKVCrossFunctionTest is Test { } /// A handle with every one of its sixteen fields in use arrives on the - /// other side of an external call bit for bit. The word it must equal is - /// computed here from the addresses the building frame was forced to - /// allocate at, so a handle that lost the word count, or a slot, or the top - /// bit of one pointer, is a different number rather than a store that - /// merely reads oddly. + /// other side of an external call bit for bit. It equals the word computed + /// here from the word count and the addresses the building frame allocates + /// its nodes at. function testSaturatedHandleCrossesTheCallBoundaryBitForBit(bytes32 seed) external view { bytes32[] memory keys = new bytes32[](LIST_COUNT); uint256 expected = (LIST_COUNT * 2) << 0xf0; diff --git a/test/src/lib/LibMemoryKV.errorAbi.t.sol b/test/src/lib/LibMemoryKV.errorAbi.t.sol index b555f74..29d0dc8 100644 --- a/test/src/lib/LibMemoryKV.errorAbi.t.sol +++ b/test/src/lib/LibMemoryKV.errorAbi.t.sol @@ -9,13 +9,9 @@ import {SetAtFreePointer} from "test/lib/SetAtFreePointer.sol"; import {COUNT_MAX, withCount} from "test/lib/LibMemoryKVHandle.sol"; /// @title LibMemoryKVErrorAbiTest -/// Every other test that expects one of these errors builds the expectation -/// from `LibMemoryKV..selector`, which is derived from the very -/// declaration under test, so adding, widening or narrowing an argument moves -/// the expectation along with it and nothing fails. A caller decoding the -/// revert off chain holds the signature as a string instead. These build the -/// expected returndata from that string, pinning each error to exactly one -/// `uint256` argument and to a selector the other error does not share. +/// Each error's revert data, expected as built from the error's signature +/// written out as a string: the first four bytes of the hash of that +/// signature, then exactly one `uint256` argument word. contract LibMemoryKVErrorAbiTest is Test, SetAtFreePointer { /// A node address low enough that it cannot be what overflows, and clear of /// the scratch space and the free memory pointer. diff --git a/test/src/lib/LibMemoryKV.exportAlloc.t.sol b/test/src/lib/LibMemoryKV.exportAlloc.t.sol index a3cd230..cc5784d 100644 --- a/test/src/lib/LibMemoryKV.exportAlloc.t.sol +++ b/test/src/lib/LibMemoryKV.exportAlloc.t.sol @@ -16,13 +16,9 @@ import {countPair} from "test/lib/LibMemoryKVExport.sol"; /// The export's ARRAY: where it is allocated, how big it is, and that every /// word of it is a key or a value that was actually inserted. /// -/// The expectations here are derived from the documented contract rather than -/// from the header the implementation writes: the README says the store -/// exports "pairwise keys/values", and `MemoryKV` is documented as carrying -/// "the total word count of all inserts", so an export of N DISTINCT keys is -/// 2N words long and occupies exactly 0x20 + 2N * 0x20 bytes of fresh memory. -/// Counting N independently of the store is the point - reading it back out of -/// `array.length` would only restate whatever the implementation wrote. +/// An export of N DISTINCT keys is 2N words long and occupies exactly +/// 0x20 + 2N * 0x20 bytes of fresh memory, with N counted by the test from the +/// keys it inserts rather than read back out of the store. contract LibMemoryKVExportAllocTest is Test { using LibMemoryKV for MemoryKV; @@ -45,10 +41,8 @@ contract LibMemoryKVExportAllocTest is Test { } /// An empty store exports an empty array and allocates the length header - /// and nothing else: exactly 0x20 bytes. The zero length is WRITTEN, not - /// inherited from memory that happened to be zero, so the header word is - /// dirtied first: an export that wrote no header would hand back the - /// sentinel as the length. + /// and nothing else: exactly 0x20 bytes. The zero length is WRITTEN: the + /// header word is dirtied first and the length reads back as zero. function testExportEmptyAllocatesOnlyTheHeader() external pure { dirtyFreeMemory(bytes32(type(uint256).max), 1); @@ -80,9 +74,7 @@ contract LibMemoryKVExportAllocTest is Test { } /// The array is allocated AT the free memory pointer and the allocation is - /// exactly the header plus two words per distinct key. Both edges matter: - /// one word short leaves the last value in unallocated memory, one word - /// long wastes a word forever. + /// exactly the header plus two words per distinct key. function testExportAllocatesExactlyHeaderPlusTwoWordsPerPair(bytes32[] memory kvs) external pure { vm.assume(kvs.length < 40); vm.assume(kvs.length % 2 == 0); @@ -128,10 +120,8 @@ contract LibMemoryKVExportAllocTest is Test { } } - /// `toBytes32Array` justifies skipping a zero-fill with "we're about to - /// write to it". Every word it allocates must therefore be written: dirty - /// the free memory first and assert the sentinel never reappears inside - /// the exported array. + /// Every word `toBytes32Array` allocates is written: with the free memory + /// dirtied first, the sentinel appears nowhere in the exported array. function testExportWritesEveryAllocatedWord(bytes32 seed) external pure { bytes32 sentinel = keccak256(abi.encode(seed, "sentinel")); @@ -190,7 +180,7 @@ contract LibMemoryKVExportAllocTest is Test { /// equality like any other. The export must copy it into the pair's first /// word and keep walking past it. The zero key is inserted LAST onto a list /// four other keys already occupy, so `set`'s prepend puts it at the head - /// and a walk that stopped at it would lose the four behind it as well. + /// and the walk reaches the four behind it through its next pointer. function testExportCopiesZeroKey(bytes32 seed) external pure { bytes32 sentinel = keccak256(abi.encode(seed, "sentinel")); bytes32 zeroKeyValue = keccak256(abi.encode(seed, "zero key value")); @@ -264,10 +254,9 @@ contract LibMemoryKVExportAllocTest is Test { } } - /// Two populated lists both land in the array: the second walk appends - /// after the first rather than writing over it. The two lists are the same - /// length so a cursor that failed to carry over would drop or duplicate a - /// pair rather than merely reorder them. + /// Two populated lists of the same length both land in the array: the + /// second walk appends after the first rather than writing over it, and + /// every pair is exported exactly once. function testExportAppendsAcrossLists(bytes32 seed) external pure { bytes32[] memory keys = new bytes32[](4); MemoryKV kv = MEMORY_KV_EMPTY; @@ -304,11 +293,8 @@ contract LibMemoryKVExportAllocTest is Test { } } - /// The export is a READ of the store. The NatSpec calls it a "one time - /// export" whose array will not reflect later mutations, which only means - /// anything if the store itself is still there afterwards: every key is - /// still gettable with its value, and a second export is identical to the - /// first. + /// The export is a READ of the store: afterwards every key is still + /// gettable with its value, and a second export is identical to the first. function testExportLeavesTheStoreIntact(bytes32 seed) external pure { bytes32[] memory keys = new bytes32[](7); MemoryKV kv = MEMORY_KV_EMPTY; diff --git a/test/src/lib/LibMemoryKV.exportWalk.t.sol b/test/src/lib/LibMemoryKV.exportWalk.t.sol index e4a8e32..87cbe1c 100644 --- a/test/src/lib/LibMemoryKV.exportWalk.t.sol +++ b/test/src/lib/LibMemoryKV.exportWalk.t.sol @@ -13,11 +13,9 @@ import {setFreePointer} from "test/lib/LibFreeMemory.sol"; /// @title LibMemoryKVExportWalkTest /// The export's WALK: following one internal list from its head to the /// terminator. `MemoryKVOverflow` bounds the HEAD pointer of a list at -/// `0xFFFF` and nothing else, because "the node's three words MAY extend above -/// `0xFFFF`, as every field is reached by full width arithmetic from the head". -/// A head near the bound therefore keeps its next word above it, and the -/// export reads that word there at full width, exactly as `get` does, to reach -/// the pairs behind that node. +/// `LibMemoryKV.POINTER_MASK` and nothing else, so a head near the bound keeps +/// its next word above it, and the export reads that word there at full width, +/// exactly as `get` does, to reach the pairs behind that node. contract LibMemoryKVExportWalkTest is Test { using LibMemoryKV for MemoryKV; @@ -36,8 +34,7 @@ contract LibMemoryKVExportWalkTest is Test { bytes32 internal constant KEY_TAIL = bytes32(uint256(4)); bytes32 internal constant KEY_HEAD = bytes32(uint256(5)); - /// Values that share no bits with either key, so a pair assembled from the - /// wrong words is a different value rather than a coincidence. + /// Values distinct from both keys and from each other. bytes32 internal constant VALUE_TAIL = bytes32(uint256(0xDEC0DE)); bytes32 internal constant VALUE_HEAD = bytes32(uint256(0xC0FFEE)); diff --git a/test/src/lib/LibMemoryKV.frameLocal.t.sol b/test/src/lib/LibMemoryKV.frameLocal.t.sol index 23628e9..26ea6a1 100644 --- a/test/src/lib/LibMemoryKV.frameLocal.t.sol +++ b/test/src/lib/LibMemoryKV.frameLocal.t.sol @@ -38,8 +38,8 @@ contract LibMemoryKVFrameLocalTest is Test { /// Read `key` out of `kv` in a frame whose memory at the node's addresses /// is exactly `filler`. A `bytes memory` argument decodes at `0x80`, its /// length word, so the three words of `filler` land at `0xA0`, `0xC0` and - /// `0xE0`, which is where the builder above put its node. Both facts are - /// required rather than assumed so a decode that moved fails loudly. + /// `0xE0`, which is where the builder above put its node. Reverts when + /// either fact does not hold. function getWithFillerExternal(MemoryKV kv, MemoryKVKey key, bytes memory filler) external pure diff --git a/test/src/lib/LibMemoryKV.gasClaims.t.sol b/test/src/lib/LibMemoryKV.gasClaims.t.sol index a104065..6ca527d 100644 --- a/test/src/lib/LibMemoryKV.gasClaims.t.sol +++ b/test/src/lib/LibMemoryKV.gasClaims.t.sol @@ -104,12 +104,9 @@ contract LibMemoryKVGasClaimsTest is Test { } /// Exporting one pair costs the same whichever slot it landed in, to within - /// `BRANCH_LAYOUT_GAS`: the bisect is the same depth to every slot but the - /// shallow one, and every branch it does not take costs the one test that - /// skips it. Delete any of those tests and the stores that skip that branch - /// pay a whole `copyFromPtr` call on an empty list, far more than - /// `BRANCH_LAYOUT_GAS`, which shows up here as a slot that no longer matches - /// its peers. + /// `BRANCH_LAYOUT_GAS`, except in the shallow slot, where it costs less: + /// the bisect is the same depth to every slot but the shallow one, and + /// every branch it does not take costs the one test that skips it. function testExportGasIsUniformAcrossSlots() public view { MemoryKV[] memory kvs = new MemoryKV[](SLOTS); for (uint256 slot = 0; slot < SLOTS; slot++) { @@ -179,8 +176,8 @@ contract LibMemoryKVGasClaimsTest is Test { MemoryKVVal value = MemoryKVVal.wrap(bytes32(uint256(2))); MemoryKVKey[] memory keys = keysInSlot(bytes32(uint256(1)), COLLIDING_SLOT, COLLIDERS); // The first key set is the one furthest from the head, so reading it - // back walks the whole list. Hoisted out of every measurement below - // because an array read inside the window is measured with the call. + // back walks the whole list. It is held in a local outside every + // measurement window below, so each window holds the call alone. MemoryKVKey first = keys[0]; MemoryKV kv = MEMORY_KV_EMPTY; diff --git a/test/src/lib/LibMemoryKV.getMatch.t.sol b/test/src/lib/LibMemoryKV.getMatch.t.sol index 25786a1..5ff6684 100644 --- a/test/src/lib/LibMemoryKV.getMatch.t.sol +++ b/test/src/lib/LibMemoryKV.getMatch.t.sol @@ -14,8 +14,7 @@ import {craftNode, handleWith, headOf, lengthOf} from "test/lib/LibMemoryKVHandl /// What `get` does with a node the walk has already reached: which bits of the /// key decide the match, and what a walk that matches nothing reports. Both /// cases put the node `get` must reject on the list the query key hashes to, -/// because a key whose list `get` never walks is rejected by the hash and says -/// nothing about the comparison. +/// so the walk reaches it. contract LibMemoryKVGetMatchTest is Test { using LibMemoryKV for MemoryKV; @@ -40,8 +39,7 @@ contract LibMemoryKVGetMatchTest is Test { /// The match is equality across the whole 256 bit word: a node key one bit /// away from the query key is a different key, whichever of the 256 bits it - /// is. A comparison narrower than the word would answer for a neighbouring - /// key over the bits it dropped, and hand back that key's value. + /// is. function testGetRequiresEveryBitOfTheKey() external pure { MemoryKVKey queryKey = MemoryKVKey.wrap(bytes32(type(uint256).max / 3)); MemoryKVVal value = MemoryKVVal.wrap(bytes32(uint256(0xDEC0DE))); @@ -68,8 +66,7 @@ contract LibMemoryKVGetMatchTest is Test { /// A key that hashes onto an OCCUPIED list but is not on it reports /// `(0, 0)`. Both returns keep their zero initialisation, so the value of a /// node the walk passed over cannot come back beside a zero `exists`. The - /// three keys are driven onto one list rather than left to collide by - /// chance, which two arbitrary keys do one time in `LibMemoryKV.LIST_COUNT`. + /// three keys are driven onto one list. function testGetMissOverAnOccupiedListReportsNoValue() external pure { MemoryKVKey head = keyForSlot(bytes32(uint256(1)), 5); MemoryKVKey tail = keyForSlot(bytes32(uint256(2)), 5); diff --git a/test/src/lib/LibMemoryKV.getScratch.t.sol b/test/src/lib/LibMemoryKV.getScratch.t.sol index e760c6c..10158a8 100644 --- a/test/src/lib/LibMemoryKV.getScratch.t.sol +++ b/test/src/lib/LibMemoryKV.getScratch.t.sol @@ -13,10 +13,8 @@ import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal, MEMORY_KV_EMPTY} from " /// assembly is declared `memory-safe`, which forbids two writes just above /// scratch: the free memory pointer at `0x40` may only move to a valid /// allocation, and the zero slot at `0x60`, the data every empty dynamic array -/// points at, may never change. A lookup that built the same hash in either -/// word instead still answers with the right value, so only the words -/// themselves say it. That `get` leaves every allocated byte above them -/// unchanged is pinned by `LibMemoryKVGetWalkTest`. +/// points at, may never change. That `get` leaves every allocated byte above +/// them unchanged is pinned by `LibMemoryKVGetWalkTest`. contract LibMemoryKVGetScratchTest is Test { using LibMemoryKV for MemoryKV; diff --git a/test/src/lib/LibMemoryKV.getWalk.t.sol b/test/src/lib/LibMemoryKV.getWalk.t.sol index 37285dd..0cac970 100644 --- a/test/src/lib/LibMemoryKV.getWalk.t.sol +++ b/test/src/lib/LibMemoryKV.getWalk.t.sol @@ -13,10 +13,9 @@ import {occupiedSlots, craftNode, handleWith, lengthOf} from "test/lib/LibMemory /// @title LibMemoryKVGetWalkTest /// `get` walks one internal list and stops at the FIRST node whose key matches. /// `set` walks the same list and also stops at the first match, so the node -/// `set` writes and the node `get` reads MUST be the same node. Nothing in the -/// public API can build a list holding one key twice, so the only way to state -/// that agreement as an observable value is to hand both functions a list that -/// does, which these tests build directly in memory. +/// `set` writes and the node `get` reads MUST be the same node. These tests +/// build a list holding one key twice directly in memory, which nothing in the +/// public API builds, and hand it to both functions. /// /// The walk is also a read: crossing a list leaves every allocated byte and the /// free memory pointer where they were. @@ -64,7 +63,6 @@ contract LibMemoryKVGetWalkTest is Test { /// `set` updates the first matching node in place. `get` MUST read that /// same node, so the value `set` just wrote is the value `get` reports. - /// If `get` kept walking it would report the stale tail node instead. function testGetAgreesWithSetOnWhichNodeIsTheKey() external pure { MemoryKVKey key = MemoryKVKey.wrap(bytes32(uint256(0xBEEF))); (MemoryKV kv, uint256 head, uint256 tail) = @@ -91,8 +89,8 @@ contract LibMemoryKVGetWalkTest is Test { /// A node deeper in the list that is NOT the head still answers, and it /// answers with its own value rather than the head's. Both keys are forced - /// onto ONE internal list, because two keys in different slots are two - /// one-node lists and never exercise the walk at all. + /// onto ONE internal list, so the walk crosses the head node to reach the + /// tail. function testGetReadsATailNodeWhenOnlyItMatches() external pure { MemoryKVKey tailKey = keyForSlot(bytes32(uint256(1)), 5); MemoryKVKey headKey = keyForSlot(bytes32(uint256(2)), 5); @@ -150,9 +148,7 @@ contract LibMemoryKVGetWalkTest is Test { } /// A hit at the tail crosses every node in front of it and leaves memory - /// exactly as it was. A walk that wrote through the pointers it follows, or - /// that allocated as it went, would corrupt or leak once per lookup while - /// still returning the right pair, so the returned pair cannot state this. + /// exactly as it was. function testGetHitLeavesMemoryUntouched() external pure { (MemoryKV kv, MemoryKVKey deepest,) = threeKeysInOneList(); diff --git a/test/src/lib/LibMemoryKV.saturate.t.sol b/test/src/lib/LibMemoryKV.saturate.t.sol index 8584407..c2a01ce 100644 --- a/test/src/lib/LibMemoryKV.saturate.t.sol +++ b/test/src/lib/LibMemoryKV.saturate.t.sol @@ -53,8 +53,7 @@ contract LibMemoryKVSaturateTest is Test { bytes32[] memory export = kv.toBytes32Array(); assertEq(export.length, kvs.length, "export length"); - // Counted over every pair at once, a pair exported twice pays for a - // pair not exported at all, so each pair is counted on its own. + // Each pair is counted on its own: exactly once. for (uint256 i = 0; i < kvs.length; i += 2) { assertEq(countPair(export, kvs[i], kvs[i + 1]), 1, "each pair exported exactly once"); } diff --git a/test/src/lib/LibMemoryKV.setCapacity.t.sol b/test/src/lib/LibMemoryKV.setCapacity.t.sol index a7354c1..7cdaad4 100644 --- a/test/src/lib/LibMemoryKV.setCapacity.t.sol +++ b/test/src/lib/LibMemoryKV.setCapacity.t.sol @@ -11,17 +11,14 @@ import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal, MEMORY_KV_EMPTY} from " /// `set` can revert, and the ceiling it reverts at is the frame's free memory /// pointer rather than a pair count. These tests state that ceiling as the /// exact pair that crosses it and the exact pointer the revert carries, and -/// state that an update is not subject to it at all, so a guard that moved -/// onto the update path or a node that changed size is a different number -/// here. +/// state that an update is not subject to it at all. contract LibMemoryKVSetCapacityTest is Test { using LibMemoryKV for MemoryKV; /// Insert `pairs` distinct keys into an empty store in a frame that starts /// at the default free memory pointer and allocates nothing else, so the /// nodes are the only allocation and their addresses are exact. Returns the - /// word count, so a fill that stopped early is a number rather than a - /// silence. + /// word count. function fillEmptyFrameExternal(uint256 pairs) external pure returns (uint256) { assembly ("memory-safe") { mstore(0x40, 0x80) @@ -35,10 +32,8 @@ contract LibMemoryKVSetCapacityTest is Test { /// Allocate an unrelated `bytes32[]` of `elements` elements in a frame that /// starts at the default free memory pointer, then insert `pairs` distinct - /// keys behind it. The array costs a length word plus its elements, and - /// that cost is required rather than assumed so a frame that started - /// somewhere else is a failure here rather than a different capacity. - /// Returns the word count. + /// keys behind it. Reverts unless the array costs exactly a length word + /// plus its elements. Returns the word count. function fillAfterUnrelatedAllocationExternal(uint256 elements, uint256 pairs) external pure returns (uint256) { assembly ("memory-safe") { mstore(0x40, 0x80) From 67bf9123134093e750a18666c1e7d84be8d6255f Mon Sep 17 00:00:00 2001 From: David Meister Date: Fri, 18 Sep 2026 15:33:02 +0000 Subject: [PATCH 2/3] docs: comments state current behaviour, part 2 Co-Authored-By: Claude Opus 5 (1M context) --- test/lib/LibMemoryKVHandle.t.sol | 3 +- test/src/lib/LibMemoryKV.bisect.t.sol | 9 ++-- test/src/lib/LibMemoryKV.exportAlloc.t.sol | 7 ++- test/src/lib/LibMemoryKV.getset.t.sol | 4 +- test/src/lib/LibMemoryKV.handleAlias.t.sol | 4 +- test/src/lib/LibMemoryKV.has.t.sol | 8 ++- test/src/lib/LibMemoryKV.setInsert.t.sol | 36 +++++-------- test/src/lib/LibMemoryKV.setSlotMask.t.sol | 18 +++---- test/src/lib/LibMemoryKV.setUpdate.t.sol | 24 ++++----- test/src/lib/LibMemoryKV.types.t.sol | 6 +-- .../lib/LibMemoryKV.wordCountOverflow.t.sol | 51 +++++++------------ 11 files changed, 64 insertions(+), 106 deletions(-) diff --git a/test/lib/LibMemoryKVHandle.t.sol b/test/lib/LibMemoryKVHandle.t.sol index 544bd63..f5bb0d9 100644 --- a/test/lib/LibMemoryKVHandle.t.sol +++ b/test/lib/LibMemoryKVHandle.t.sol @@ -79,8 +79,7 @@ contract LibMemoryKVHandleTest is Test { } /// A node at `LibMemoryKV.POINTER_MASK` still fits a head slot; one byte - /// higher does not and reverts rather than handing back an address a handle - /// would truncate. + /// higher does not, and crafting a node there reverts. function testCraftNodeRevertsAboveTheWidestHeadPointer() external { assertEq( this.craftNodeAtExternal(LibMemoryKV.POINTER_MASK), LibMemoryKV.POINTER_MASK, "the widest head pointer" diff --git a/test/src/lib/LibMemoryKV.bisect.t.sol b/test/src/lib/LibMemoryKV.bisect.t.sol index c21b39c..879b420 100644 --- a/test/src/lib/LibMemoryKV.bisect.t.sol +++ b/test/src/lib/LibMemoryKV.bisect.t.sol @@ -392,8 +392,8 @@ contract LibMemoryKVBisectTest is Test { /// Exports all 32768 occupancy combinations and checks each against the /// pairs that combination must produce. The check is plain arithmetic and /// the first mask that fails it is carried out of the loop, so the assertion - /// machinery runs once rather than 32768 times and the mask that failed is - /// what the assertion reports. + /// machinery runs once and the mask that failed is what the assertion + /// reports. /// /// Every case is built from one free memory pointer, `free`, taken after /// `slotKeys` has allocated, so every node of every case lies in @@ -459,8 +459,9 @@ contract LibMemoryKVBisectTest is Test { /// The length is not a slot. It sits directly above slot 14, which is the /// leaf the collapsed path reaches without scrubbing, and the export reads - /// no pointer out of it. Every pair here is on one low list, which leaves the whole high half - /// empty while the length is far larger than any single insert makes it. + /// no pointer out of it. Every pair here is on one low list, which leaves + /// the whole high half empty while the length is far larger than any single + /// insert makes it. /// /// It is also the only list built longer than one node, so it is the only /// place the walk down a list is exercised at all. Which order it comes back diff --git a/test/src/lib/LibMemoryKV.exportAlloc.t.sol b/test/src/lib/LibMemoryKV.exportAlloc.t.sol index cc5784d..cc9a563 100644 --- a/test/src/lib/LibMemoryKV.exportAlloc.t.sol +++ b/test/src/lib/LibMemoryKV.exportAlloc.t.sol @@ -155,10 +155,9 @@ contract LibMemoryKVExportAllocTest is Test { } /// A value of `0` is a value: `get` documents that any key MAY be set to - /// zero and reports existence separately from it. The export must write - /// that zero into the pair's second word rather than leave the word as - /// whatever memory held, so the free memory is dirtied first and the - /// assertion is on the zero itself. + /// zero and reports existence separately from it. The export writes that + /// zero into the pair's second word: the free memory is dirtied first, and + /// the assertion is on the zero itself. function testExportCopiesZeroValue(bytes32 seed) external pure { bytes32 sentinel = keccak256(abi.encode(seed, "sentinel")); bytes32 zeroValued = keccak256(abi.encode(seed, "zero valued")); diff --git a/test/src/lib/LibMemoryKV.getset.t.sol b/test/src/lib/LibMemoryKV.getset.t.sol index b3eb996..a6bd53c 100644 --- a/test/src/lib/LibMemoryKV.getset.t.sol +++ b/test/src/lib/LibMemoryKV.getset.t.sol @@ -63,9 +63,7 @@ contract LibMemoryKVGetSetTest is Test, SetAtFreePointer { /// The bound is on the node's HEAD pointer alone. A node inserted at the /// maximum head pointer `0xFFFF` holds its value word at `0x1001F`, above /// the bound, and `get` reads it: node fields are reached by full width - /// arithmetic from the head, not through 16 bit pointers. A `get` that - /// truncated a field address to 16 bits is indistinguishable from a - /// correct one for every node that fits entirely under the bound. + /// arithmetic from the head, not through 16 bit pointers. function testGetReadsAValueWordAboveTheBound(MemoryKVKey key, MemoryKVVal value) external view { (uint256 exists, MemoryKVVal got) = this.setAtPointerThenGetExternal(key, value, 0xFFFF); diff --git a/test/src/lib/LibMemoryKV.handleAlias.t.sol b/test/src/lib/LibMemoryKV.handleAlias.t.sol index f5b9898..0cd2c0d 100644 --- a/test/src/lib/LibMemoryKV.handleAlias.t.sol +++ b/test/src/lib/LibMemoryKV.handleAlias.t.sol @@ -15,9 +15,7 @@ import {assertValue} from "test/lib/LibMemoryKVAssert.sol"; /// tests pin what those copies report: an update reaches every handle holding /// the key, an insert reaches only the handle `set` returned, and /// `toBytes32Array` is the only way to hold values that a later update cannot -/// move. Each case states the VALUE a handle must report, so a store that -/// started copying on write, or one that stopped sharing items, is a different -/// number rather than a revert. +/// move. Each case states the VALUE a handle must report. contract LibMemoryKVHandleAliasTest is Test { using LibMemoryKV for MemoryKV; diff --git a/test/src/lib/LibMemoryKV.has.t.sol b/test/src/lib/LibMemoryKV.has.t.sol index a9200e7..752f95c 100644 --- a/test/src/lib/LibMemoryKV.has.t.sol +++ b/test/src/lib/LibMemoryKV.has.t.sol @@ -10,8 +10,7 @@ import {LibMemoryKV, MemoryKV, MemoryKVKey, MemoryKVVal, MEMORY_KV_EMPTY} from " /// @title LibMemoryKVHasTest /// `has` answers the existence half of `get`, so every case here states what -/// `get` reports and what `has` must therefore say, rather than repeating how -/// the walk finds it. +/// `get` reports and what `has` must therefore say. contract LibMemoryKVHasTest is Test { using LibMemoryKV for MemoryKV; @@ -28,9 +27,8 @@ contract LibMemoryKVHasTest is Test { assertFalse(kv.has(other)); } - /// A key SET TO ZERO exists. This is the case a caller gets wrong by - /// reading the value instead of the existence flag, and the reason the two - /// are reported separately. + /// A key SET TO ZERO exists: `has` is true, and `get` reports the key as + /// existing with a zero value. function testHasAKeyWhoseValueIsZero(MemoryKVKey key) external pure { MemoryKV kv = MEMORY_KV_EMPTY.set(key, MemoryKVVal.wrap(0)); assertTrue(kv.has(key)); diff --git a/test/src/lib/LibMemoryKV.setInsert.t.sol b/test/src/lib/LibMemoryKV.setInsert.t.sol index de5bcba..d09ef13 100644 --- a/test/src/lib/LibMemoryKV.setInsert.t.sol +++ b/test/src/lib/LibMemoryKV.setInsert.t.sol @@ -13,8 +13,8 @@ import {keysInSlot} from "test/lib/LibMemoryKVKeys.sol"; import {headOf, lengthOf} from "test/lib/LibMemoryKVHandle.sol"; /// @title LibMemoryKVSetInsertTest -/// The insert half of `set`, asserted against the documented SHAPE of the store -/// rather than against a round trip through `get`. +/// The insert half of `set`, asserted against the documented SHAPE of the +/// store. /// /// The layout is the one the `MemoryKV` NatSpec documents and the `LibMemoryKV` /// constants name. So an insert must place a `LibMemoryKV.NODE_BYTES` @@ -35,11 +35,6 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { /// An insert into an empty store writes exactly three words AT the free /// memory pointer -- key, then value, then the old head of the list -- and /// records that same address as the list head, with a count of two. - /// - /// Stated as exact addresses and words rather than as "the value comes back - /// out", so a node written next to the free memory pointer, a slot holding - /// an address the node is not at, or a next word that is not the old head - /// is a different NUMBER here, not just a failed lookup. function testSetInsertWritesThreeWordsAtTheFreeMemoryPointer(MemoryKVKey key, MemoryKVVal value) external pure { MemoryKV kv = MEMORY_KV_EMPTY; @@ -57,10 +52,9 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { assertEq(next, 0, "next at node+0x40 is the old (empty) head"); } - /// An insert WRITES all three words of its node. A zero key, a zero value - /// and the terminator of an empty list are what the node HOLDS, not what - /// the memory under it happened to hold: the node lands on a sentinel here, - /// so a word the insert leaves alone reads back as that sentinel instead. + /// An insert WRITES all three words of its node: the node lands on memory + /// dirtied with a sentinel, and its words read back as the zero key, the + /// zero value and the terminator of an empty list. function testSetInsertWritesEveryWordOfTheNode(bytes32 seed) external pure { bytes32 sentinel = keccak256(abi.encode(seed)); MemoryKVKey key = MemoryKVKey.wrap(bytes32(0)); @@ -83,8 +77,7 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { /// Three keys that share one list: each insert PREPENDS, so the head is the /// newest node and every older node is still reachable behind it. Asserts - /// the chain of addresses, which is the fact that "nothing inserted is - /// lost" rests on. + /// the chain of addresses. function testSetInsertPrependsWithinOneList() external pure { MemoryKVKey[] memory keys = keysInSlot(bytes32(uint256(1)), 0, 3); MemoryKV kv = MEMORY_KV_EMPTY; @@ -99,8 +92,8 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { assertEq(node1, node0 + LibMemoryKV.NODE_BYTES, "second node follows the first"); assertEq(node2, node1 + LibMemoryKV.NODE_BYTES, "third node follows the second"); - // The head is the newest node, and ONLY the newest -- the old head is - // masked out of the slot rather than ored together with the new one. + // The head is the newest node, and ONLY the newest: the old head is + // masked out of the slot. assertEq(headOf(kv, keys[0]), node2, "head is the newest node"); assertEq(lengthOf(kv), 6, "three pairs is six words"); @@ -132,9 +125,8 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { /// The word count's slot is `LibMemoryKV.SLOT_BITS` wide, so it keeps /// counting past 0xFF. 200 pairs is 400 words, which does not fit in a - /// byte; a count that wrapped at 256 would both report the wrong number - /// here and make `toBytes32Array` (which preallocates from it) return a - /// short array. + /// byte: the count reads 400, and `toBytes32Array`, which preallocates from + /// it, exports 400 words. function testSetInsertWordCountPastAByte() external pure { uint256 pairs = 200; MemoryKV kv = MEMORY_KV_EMPTY; @@ -155,8 +147,7 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { } /// The overflow error carries the OFFENDING pointer, not the bound it - /// crossed. `0x10000` is both the first invalid pointer and the value one - /// past the bound, so it cannot tell the two apart; `0x12345` can. + /// crossed. `0x12345` is neither the bound nor one past it. function testSetOverflowPayloadIsTheOffendingPointerNotTheBound() external { MemoryKVKey key = MemoryKVKey.wrap(bytes32(uint256(1))); MemoryKVVal value = MemoryKVVal.wrap(bytes32(uint256(2))); @@ -174,9 +165,8 @@ contract LibMemoryKVSetInsertTest is Test, SetAtFreePointer { assertEq(lengthOf(kv), 2, "one pair is two words"); } - /// The head pointer an insert produces exists ONLY in the returned store, - /// which is why `set` documents that the return MUST be assigned back. The - /// node is allocated and written either way, so a caller that drops the + /// The head pointer an insert produces exists ONLY in the returned store. + /// The node is allocated and written either way, so a caller that drops the /// return is left holding the word it already had and the pair is reachable /// from nothing -- no revert, no short array, just a missing key. function testSetInsertIsUnreachableWhenTheReturnIsDropped( diff --git a/test/src/lib/LibMemoryKV.setSlotMask.t.sol b/test/src/lib/LibMemoryKV.setSlotMask.t.sol index 3228ced..c31cbf2 100644 --- a/test/src/lib/LibMemoryKV.setSlotMask.t.sol +++ b/test/src/lib/LibMemoryKV.setSlotMask.t.sol @@ -15,19 +15,15 @@ import {headOf, lengthOf} from "test/lib/LibMemoryKVHandle.sol"; /// keeps no bit of the old one, neither the slot's low bit nor its high bit, /// and the slot above keeps every bit of its own head, its low bit included. /// -/// A Solidity allocator places every node at a multiple of 32, and every newer -/// node above every older one. The low bits of an old and a new head are then -/// both zero, and an old head with the high bit set means a new head with it -/// set too, so neither edge of the slot is exercised. These tests place nodes -/// at addresses the library allows but an allocator would not choose: an odd -/// one, and an older one with the high bit set under a newer one without it, -/// in the same list and in neighbouring lists. A node address is a caller's -/// free memory pointer, and `set` bounds it by magnitude alone. +/// These tests place nodes at addresses `set` accepts but a Solidity allocator +/// does not choose: an odd one, and an older one with the high bit set under a +/// newer one without it, in the same list and in neighbouring lists. A node +/// address is a caller's free memory pointer, and `set` bounds it by magnitude +/// alone. contract LibMemoryKVSetSlotMaskTest is Test, SetAtFreePointer { using LibMemoryKV for MemoryKV; - /// An odd node address, low enough that a walk off the end of it expands - /// memory by kilobytes rather than megabytes. + /// An odd node address. uint256 internal constant ODD_POINTER = 0x101; /// An even node address clear of the three words at `ODD_POINTER`, with @@ -112,7 +108,7 @@ contract LibMemoryKVSetSlotMaskTest is Test, SetAtFreePointer { /// store) included, so list 14 is still headed at `ODD_POINTER`. /// /// List 14 is the highest there is, so its head sits directly under the - /// word count and is the slot a rewrite is likeliest to reach past. + /// word count. function testInsertLeavesTheNeighbouringSlotsLowBit() external view { MemoryKVKey high = keyForSlot(bytes32(uint256(1)), 14); MemoryKVKey low = keyForSlot(bytes32(uint256(1)), 13); diff --git a/test/src/lib/LibMemoryKV.setUpdate.t.sol b/test/src/lib/LibMemoryKV.setUpdate.t.sol index e1a3d48..15e7160 100644 --- a/test/src/lib/LibMemoryKV.setUpdate.t.sol +++ b/test/src/lib/LibMemoryKV.setUpdate.t.sol @@ -15,17 +15,14 @@ import {setFreePointer} from "test/lib/LibFreeMemory.sol"; /// @title LibMemoryKVSetUpdateTest /// `set` hashes the key to one of `LibMemoryKV.LIST_COUNT` internal lists, /// walks that list for a match, and on a match mutates the value in place. -/// Every case here states the VALUE the store must report afterwards, so a walk -/// that finds the wrong node, a hash that disagrees with `get`, or an "update" -/// that allocates shows up as a different number rather than as a revert. +/// Every case here states the VALUE the store must report afterwards. contract LibMemoryKVSetUpdateTest is Test { using LibMemoryKV for MemoryKV; /// `set` must hash into the same list `get` reads from, for EVERY one of the /// `LibMemoryKV.LIST_COUNT` lists. The head pointer the store ends up - /// holding names the list, so a set that hashed differently (different - /// preimage, different modulus, different bit stride) parks the pointer in - /// the wrong slot and the value is no longer readable. + /// holding names the list: it is in the slot the key hashes to and no + /// other, and `get` reads the value back. function testSetHashesIntoTheSameListGetReads() external pure { for (uint256 slot = 0; slot < LibMemoryKV.LIST_COUNT; slot++) { MemoryKVKey key = keyForSlot(bytes32(slot + 1), slot); @@ -67,9 +64,8 @@ contract LibMemoryKVSetUpdateTest is Test { } /// Updating one key of a list of colliding keys must land on that key's own - /// node. The three keys below share one internal list, so a walk that never - /// runs, or that compares the wrong word, writes the value onto whichever - /// node it stopped at instead. + /// node. The three keys below share one internal list, and the update + /// through the middle key changes the middle value alone. function testUpdateFindsTheRightNodeAmongColliders() external pure { MemoryKVKey tail = keyForSlot(bytes32(uint256(0xA)), 0x07); MemoryKVKey middle = keyForSlot(bytes32(uint256(0xB)), 0x07); @@ -208,9 +204,8 @@ contract LibMemoryKVSetUpdateTest is Test { } /// The word count the store carries must stay equal to twice the number of - /// distinct keys, however many times those keys are rewritten. A count that - /// drifted up on updates would make the export allocate and report slots the - /// linked lists never fill. + /// distinct keys, however many times those keys are rewritten, and the + /// export is that many words long. function testWordCountCountsDistinctKeysOnly(MemoryKVKey[] memory keys, MemoryKVVal value) external pure { vm.assume(keys.length <= 20); MemoryKV kv = MEMORY_KV_EMPTY; @@ -237,9 +232,8 @@ contract LibMemoryKVSetUpdateTest is Test { /// The match compares the whole 256 bit key word. Each pair below shares /// one internal list and differs in a single bit, at the top and at the - /// bottom of the word, so a comparison narrowed at either end would stop at - /// the first key's node and overwrite its value instead of inserting the - /// second key, leaving one pair where there must be two. + /// bottom of the word, and setting the second key inserts it: two pairs, + /// each key with its own value. function testSetDistinguishesKeysDifferingInOneBit() external pure { uint256[2] memory bits = [uint256(0), 0xff]; for (uint256 i = 0; i < bits.length; i++) { diff --git a/test/src/lib/LibMemoryKV.types.t.sol b/test/src/lib/LibMemoryKV.types.t.sol index 86bb3f4..a978a36 100644 --- a/test/src/lib/LibMemoryKV.types.t.sol +++ b/test/src/lib/LibMemoryKV.types.t.sol @@ -69,10 +69,8 @@ contract LibMemoryKVTypesTest is Test { } } - /// Two keys differing in the top bit alone, sharing one internal list. A - /// compare narrower than a word merges them: one pair counted instead of - /// two, and one value answering for both. No fuzzed pair of keys differs in - /// that bit alone, so the pair is searched for rather than drawn. + /// Two keys differing in the top bit alone, sharing one internal list, are + /// two keys: two pairs counted, and each key answering with its own value. function testTopBitCollidersAreTwoKeys() external pure { (MemoryKVKey low, MemoryKVKey high) = collidingPairDifferingInBit(0, TOP_BIT); assertTwoKeys(low, high); diff --git a/test/src/lib/LibMemoryKV.wordCountOverflow.t.sol b/test/src/lib/LibMemoryKV.wordCountOverflow.t.sol index 59fba0f..29652bf 100644 --- a/test/src/lib/LibMemoryKV.wordCountOverflow.t.sol +++ b/test/src/lib/LibMemoryKV.wordCountOverflow.t.sol @@ -11,11 +11,8 @@ import {setFreePointer} from "test/lib/LibFreeMemory.sol"; /// @title LibMemoryKVWordCountOverflowTest /// The word count is SIXTEEN bits and an insert adds two to it, so there is a -/// count from which one more insert does not fit. The count is written by -/// shifting into the top of `MemoryKV`, which drops whatever does not fit, and -/// `toBytes32Array` sizes its allocation from the count and then copies every -/// pair it walks -- so a count that lost its high bit is a short allocation -/// written past, not a wrong number a caller might notice. +/// count from which one more insert does not fit, and `set` reverts +/// `MemoryKVLengthOverflow` for that insert. /// /// A store built only through `set` cannot get there, because a node address /// must fit sixteen bits too and that runs out first. That is a bound on @@ -48,9 +45,8 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { return (kv, exists, MemoryKVVal.unwrap(value)); } - /// `0xFFFE + 2` is `0x10000`, which is one bit wider than the field. Shifted - /// into place that bit is gone and the count reads back as `0`, so the - /// insert that does not fit is the one that reports an empty store. + /// `0xFFFE + 2` is `0x10000`, which is one bit wider than the field, and the + /// insert reverts carrying `0x10000`. function testSetRevertsRatherThanWrappingTheWordCountToZero() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVLengthOverflow.selector, 0x10000)); this.setAtFreePointer( @@ -76,8 +72,7 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { } /// The last insert that fits still happens. `0xFFFC + 2` is `0xFFFE`, the - /// widest even count the field holds, and it must come back intact rather - /// than be refused a step early. + /// widest even count the field holds, and it is written whole. function testSetAcceptsTheWidestWordCountThatFits() external view { MemoryKVKey key = MemoryKVKey.wrap(bytes32(uint256(1))); MemoryKV kv = this.setAtFreePointer( @@ -107,8 +102,7 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { } /// When the node address and the count both overflow at once the address is - /// what gets reported: it is the one that has already corrupted the list, - /// and the count is only the size of the export that would have followed. + /// what gets reported. function testSetReportsThePointerWhenBothOverflow() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVOverflow.selector, 0x12345)); this.setAtFreePointer( @@ -119,11 +113,10 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { ); } - /// `MemoryKVLengthOverflow` is documented for a count pushed "past - /// `0xFFFF`", so `0xFFFF` itself still fits and must be written whole. It is - /// also the widest value the shared comparison accepts, reached here with - /// both of its inputs nonzero, so a guard that added the node address and - /// the count rather than ORing their bits would refuse this insert. + /// `MemoryKVLengthOverflow` is documented for a word count pushed "past + /// `POINTER_MASK`", so a count of exactly `0xFFFF` still fits and is written + /// whole. It is also the widest value the shared comparison accepts, + /// reached here with both of its inputs nonzero. function testSetAcceptsAWordCountOfExactlyTheBound() external view { MemoryKVKey key = MemoryKVKey.wrap(bytes32(uint256(1))); MemoryKV kv = this.setAtFreePointer( @@ -134,11 +127,9 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { assertEq(headOf(kv, key), LOW_FREE_POINTER, "the node is still recorded"); } - /// `MemoryKVOverflow` is documented for a node address "above `0xFFFF`", so - /// a node landing exactly on `0xFFFF` has not overflowed and an overflowing - /// count alongside it is still reported as the count's error. Every other - /// input either leaves the guard alone or is already past the address bound, - /// so this is what separates an address bound of `above` from one of `at`. + /// `MemoryKVOverflow` is documented for a node "above `POINTER_MASK`", so a + /// node landing exactly on `0xFFFF` has not overflowed and an overflowing + /// count alongside it is still reported as the count's error. function testSetReportsTheCountWhenTheAddressIsTheWidestValidOne() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVLengthOverflow.selector, 0x10000)); this.setAtFreePointer( @@ -151,10 +142,8 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { /// The bound is `0xFFFF`, not the `0x10000` that first crosses it. With the /// node address and the count both at `0x10000` their combined - /// `pointer | length` is `0x10000` as well, so a bound written one too high - /// lets this insert through entirely rather than merely naming the wrong - /// overflow: `0x12345` above carries bits a raised bound still catches, and - /// this carries none. + /// `pointer | length` is `0x10000` as well, and the insert reverts + /// `MemoryKVOverflow` carrying the node address. function testSetOverflowsWhenBothValuesAreTheFirstInvalidOne() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVOverflow.selector, 0x10000)); this.setAtFreePointer( @@ -177,12 +166,10 @@ contract LibMemoryKVWordCountOverflowTest is Test, SetAtFreePointer { } /// Every other test here forces the count with `withCount`, which is a - /// `kv` no `set` produced. This is the one that asks what a caller who only - /// ever calls `set` can reach, and the answer is that it is not this error: - /// carrying the count to `0x10000` needs 32768 inserts, and the node - /// address runs out 48 times earlier, at the 683rd, at `0x10040`. So - /// `MemoryKVLengthOverflow` is not in a correct caller's reach and the - /// header's claim above is a measurement rather than an assurance. + /// `kv` no `set` produced. This one fills an empty store through `set` + /// alone: carrying the count to `0x10000` needs 32768 inserts, and the node + /// address runs out 48 times earlier, at the 683rd, at `0x10040`, so the + /// fill reverts `MemoryKVOverflow`, not `MemoryKVLengthOverflow`. function testWordCountBoundIsUnreachableFromAnEmptyStore() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVOverflow.selector, 0x10040)); this.fillFromEmptyExternal((COUNT_MAX + 1) / 2); From aab697bc9f88ed834c1ef85e64fb120563299320 Mon Sep 17 00:00:00 2001 From: David Meister Date: Fri, 18 Sep 2026 17:15:30 +0000 Subject: [PATCH 3/3] docs: state current behaviour at the sites merged PRs held #123, #173 and #136/#115/#158 are merged, so the comments this sweep left to them are in its scope again. COLLIDING_SLOT drops "any list would do", PAIRS_PER_STORE states where ten pairs put the nodes instead of what an uncapped array does, and the two setCapacity tests drop the pair-count argument and keep the fact it rests on. Co-Authored-By: Claude Opus 5 (1M context) --- test/src/lib/LibMemoryKV.gasClaims.t.sol | 5 ++--- test/src/lib/LibMemoryKV.setCapacity.t.sol | 8 +++----- test/src/lib/LibMemoryKV.storageParity.t.sol | 6 ++---- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/test/src/lib/LibMemoryKV.gasClaims.t.sol b/test/src/lib/LibMemoryKV.gasClaims.t.sol index a68ffc0..2424835 100644 --- a/test/src/lib/LibMemoryKV.gasClaims.t.sol +++ b/test/src/lib/LibMemoryKV.gasClaims.t.sol @@ -31,9 +31,8 @@ contract LibMemoryKVGasClaimsTest is Test { /// How many keys the colliding measurements put into one list. uint256 internal constant COLLIDERS = 4; - /// The list the colliding measurements build. Any list would do: an insert - /// into an empty list and a get of a key alone in one cost the same in - /// every slot. + /// The list the colliding measurements build. An insert into an empty list + /// and a get of a key alone in one cost the same in every slot. uint256 internal constant COLLIDING_SLOT = 3; /// Expands memory past anything the measurements below allocate, then diff --git a/test/src/lib/LibMemoryKV.setCapacity.t.sol b/test/src/lib/LibMemoryKV.setCapacity.t.sol index 758903b..6c2cf0b 100644 --- a/test/src/lib/LibMemoryKV.setCapacity.t.sol +++ b/test/src/lib/LibMemoryKV.setCapacity.t.sol @@ -77,8 +77,7 @@ contract LibMemoryKVSetCapacityTest is Test { /// One pair past `EMPTY_FRAME_PAIRS` would start at /// `0x80 + EMPTY_FRAME_PAIRS * NODE_BYTES`, past the widest pointer a slot - /// holds, so it reverts carrying that address. This is the ceiling as a - /// pair count, which is the form a caller can measure itself against. + /// holds, so it reverts carrying that address. function testSetOverflowsOnePairPastAnOtherwiseEmptyFramesCapacity() external { vm.expectRevert( abi.encodeWithSelector( @@ -97,9 +96,8 @@ contract LibMemoryKVSetCapacityTest is Test { /// The `EMPTY_FRAME_PAIRS` pairs that fit an otherwise empty frame revert /// once one unrelated word is in that frame: the last of them would start - /// at `0x10000`. A pair count is therefore not a capacity: what the caller - /// has already allocated decides whether the same count succeeds or - /// reverts. + /// at `0x10000`. What the caller has already allocated decides whether the + /// same pair count succeeds or reverts. function testOneUnrelatedWordMakesAnEmptyFramesCapacityOverflow() external { vm.expectRevert(abi.encodeWithSelector(LibMemoryKV.MemoryKVOverflow.selector, 0x10000)); this.fillAfterUnrelatedAllocationExternal(0, EMPTY_FRAME_PAIRS); diff --git a/test/src/lib/LibMemoryKV.storageParity.t.sol b/test/src/lib/LibMemoryKV.storageParity.t.sol index a1da57b..c00caa0 100644 --- a/test/src/lib/LibMemoryKV.storageParity.t.sol +++ b/test/src/lib/LibMemoryKV.storageParity.t.sol @@ -24,10 +24,8 @@ contract LibMemoryKVStorageParityTest is Test { /// How many pairs each store in `testTwoLiveStoresEachMatchTheirOwnStorage` /// takes from its fuzzed array. Both decoded arrays sit in memory below the - /// first node, a pointer word and two struct words per pair, so uncapped - /// arrays can push the nodes past `POINTER_MASK` and `set` reverts - /// `MemoryKVOverflow` instead of the test checking parity. Ten per store - /// keeps every node below it. + /// first node, a pointer word and two struct words per pair, and with ten + /// pairs per store every node sits below `POINTER_MASK`. uint256 internal constant PAIRS_PER_STORE = 10; //forge-lint: disable-next-line(mixed-case-variable)