Check the hex charset in LibHexString.bytesToHex

[thedavidmeister]

Closes#68

The call: tighten the code, not the docs

#68 offers two routes — check the hexadecimal charset the docs promise, or narrow
the two docstrings to the length-and-prefix check the code actually performs.
Tightened the code.

The charset is not decoration on this function. Its entire job is to produce the
payload of a hex"..." literal in generated Solidity, and the two failure modes
the guard already covered are the same class as the one it did not:

Vm returns	before	after
shorter than 2n+2	reverts (would underflow the length word)	reverts
right length, no 0x	reverts (would discard two bytes of real data)	reverts
right length, 0x, payload not hex	accepted, reaches generated source as hex"ZZZZ" which does not compile and blames the generated file, not the Vm	reverts

The suite already asserted the charset as a property of the output
(testBytesToHexCharset, with the rationale "Anything else in the string
terminates or corrupts the hex"..." literal it is spliced into"), and both
docstrings already claimed it. The implementation was the only place that did
not, and the fuzz oracle encoded the gap rather than the definition it says it
derives from.

Ledger

Tighten (taken)

• Build: 16 lines in LibHexString.bytesToHex (a bounds-checked scan plus one
  clause on the existing strip gate), 9 tests, 7 lines in the fuzz oracle.
• Carrying: the scan runs once per call and costs roughly 200 gas per emitted
  character — testBytesToHexLongData (300 bytes in, 600 characters out) moves
  44,931 → 169,939 gas, testBytesToHexKnown 4,904 → 7,686. bytesToHex calls
  vm.toString, so it only ever executes against the cheatcode address inside a
  forge test or script: that gas is wall-clock on a codegen run, never gas anyone
  pays. The file already states this tradeoff explicitly for its revert path
  ("a build-time function where gas is not a consideration"), which is also why
  the scan is written in bounds-checked Solidity rather than hand-rolled Yul.
• Removal later: delete the scan, the gate clause, the 9 tests and the oracle
  clause. UnexpectedHexString keeps its signature and no caller, storage layout
  or interface depends on the new behaviour, so removal is local to one file plus
  its test.
• Behaviour change: a Vm returning the right length and prefix with a
  non-lower-case-hex payload now reverts instead of returning. Foundry's own Vm
  never does this, and the only in-tree caller is
  LibCodeGen.bytesConstantString, which is unaffected.

Narrow the docs (declined)

• Build: ~3 lines.
• Carrying: leaves a guard that catches two of the three ways a Vm return can
  poison generated source and waves the third through; leaves
  testBytesToHexCharset's stated rationale enforced on the output but not on
  the input it is derived from; and leaves the fuzz property named in LibHexString.bytesToHex documents a hex-charset guarantee it does not check, and the fuzz oracle encodes the same gap #68 stating
  it derives conformance from the definition of toString(bytes) while encoding
  something weaker. Nothing about that gets cheaper with time.
• Removal later: nothing to remove, but the finding returns the next time
  anyone reads the two docstrings against the code.

The docstrings are still touched, because the old wording overclaimed in a second
way the issue does not raise: it said the Vm must "return the string that
toString(bytes) is defined to return". No shape check can establish that — a
Vm returning 0xdeadbeef for hex"aabb" is the right length, prefix and
charset and is still the wrong string, and telling that apart means redoing the
conversion the Vm was called for. The docs now say the shape is what is
checked, and say so out loud.

Where the issue's proposed fix was changed

• The proposal put the scan in the memory-safe assembly block, reading
  byte(0, mload(add(add(hexString, 0x20), i))). That is correct but it does a
  full word load per character, and for a payload whose length is a multiple of
  32 the final loads run past the string's own allocation into memory after the
  free memory pointer. Solidity's bounds-checked indexing in the function body
  is the same check with none of that, and gas is not a consideration here.
• The scan therefore runs before the assembly block, not after the prefix
  check, and its result gates the strip via and(hexCharset, ...). Order
  matters: the strip mutates the string in place, so a charset check after it
  would put the stripped string into UnexpectedHexString, contradicting
  @param hexString The string the Vm returned.
• The proposal's charset test only covers 0-9 and a-f, which is right;
  this PR pins the four range boundaries in both directions (rejects /, :,
  a backtick and g; accepts 0, 9, a, f) rather than only spot-checking
  one non-hex character.

The interaction with #102, measured

#102 (issue #59) has landed. It renamed the fuzz property this PR extends to
testBytesToHexStripsOrRevertsForEveryVmOutput(bytes,string,bool) and gave it a
CONSTRUCTED accept arm, because an unconstructed Vm return conforms 0 times in
2048 runs. This PR adds a charset clause to the conforms predicate inside that
same function and changes nothing else in it.

An earlier revision of this description predicted, without measuring, that "with
this PR merged, an arbitrary-byte payload fails the charset check almost always, so
#102's accept arm collapses back toward the 0 reaches per 2048 it exists to fix."
Measured, it does not. A per-run counter on conforms,
2048 runs, the same three seeds, on main (89cb0a2) and on this head:

seed	accept arm on main	accept arm here	unconstructed half conforming, main / here
1	1021 / 2048	56 / 2048	0 / 0
2	991 / 2048	49 / 2048	0 / 0
3	1036 / 2048	53 / 2048	0 / 2

The main column reproduces #102's own figures exactly — #102 recorded
1021/991/1036/984 per 2048 for seeds 1-4 — so this counter is measuring the same
thing #102 measured, not a different quantity. The accept arm survives because two constructions still pass
the charset: an empty filler, for which #102's loop fills the payload with the
literal "a", and a filler whose used bytes all land in 0-9/a-f.

So the property still exercises both halves, but the accepted half is reached
about 2.6% of runs rather than about 50%. Mapping filler into 0-9/a-f
would restore the even split. That belongs to #102's diff, not this one, and is
not applied here.

Two smaller findings from the same measurement:

• The unconstructed half is no longer strictly 0. The hand-written cases this PR
  adds put conforming string literals such as "0x0123456789abcdef" into the
  compiled artifacts, and forge seeds the fuzz dictionary from those, so a raw
  filler now conforms occasionally (2 of 2048 at seed 3).
• One line of test: construct the conforming half of the bytesToHex Vm-output property #102's docstring is corrected in this PR, because this PR is what
  falsifies it: it said the accepted string is "arbitrary in everything except
  the length and prefix the library actually checks", and the library now checks
  the charset too. The construction itself is untouched.

QA

• Discriminating tests: testBytesToHexRevertsOnNonHexVmOutput, testBytesToHexRevertsOnUpperCaseVmOutput, testBytesToHexRevertsOnNonHexFirstPayloadCharacter, testBytesToHexRevertsOnNonHexLastPayloadCharacter, testBytesToHexRevertsOnCharacterBelowDigitRange, testBytesToHexRevertsOnCharacterAboveDigitRange, testBytesToHexRevertsOnCharacterBelowLetterRange, testBytesToHexRevertsOnCharacterAboveLetterRange, plus the fuzz property — each fails with the tests kept and src/lib/LibHexString.sol reverted to main's version, transcript in section 1. testBytesToHexAcceptsEveryHexNibble passes against main's library by design; it is the over-tightening guard, and it is what kills M9-M12.
• Oracle: the charset rule comes from the definition of foundry's Vm.toString(bytes) (two lower case hexadecimal nibbles per input byte) and from what a hex"..." literal in generated Solidity will accept — not read back off LibHexString. The hand-written cases assert against literal expected strings and the four range boundaries chosen from ASCII (/: backtick g reject; 09af accept), and the fuzz property recomputes conformance in Solidity from that same definition rather than calling the library twice.
• Category check: LibHexString.bytesToHex documents a hex-charset guarantee it does not check, and the fuzz oracle encodes the same gap #68 asks for two things — (A) the charset guarantee the docstrings claim but the code does not check, and (B) the fuzz oracle that encodes the same gap; covered A (scan + gate in bytesToHex, 9 tests) and B (charset clause in conforms, mutation M13). LibHexString.bytesToHex documents a hex-charset guarantee it does not check, and the fuzz oracle encodes the same gap #68 also offers the alternative of narrowing the docs instead; declined with the ledger above, and the docstrings are still corrected because their old wording overclaimed in a way no shape check can deliver.

Everything below was re-run with nix develop -c from the flake at eb344aa,
which merges main at 89cb0a2 (post-#56, post-#102, post-#126, post-#138).
The transcripts this section used to carry were run against
test/lib/LibHexString.bytesToHex.t.sol and forge-std-1.16.1, neither of which
exists now, and are replaced rather than kept. Every restore in the harness is
git restore --staged --worktree, because git checkout <ref> -- FILE also
writes the index and a later plain git checkout -- FILE then silently restores
the wrong version; the harness also asserts which version of each file is on disk
before every measurement. The persisted fuzz corpus is deleted before every run,
so no kill below is a replay of an earlier row's counterexample.
Scripts: pr-108-failfirst.sh, pr-108-mutate.sh, pr-108-mutate-names.sh,
pr-108-verify.sh.

1. Failing first — tests kept, src/lib/LibHexString.sol reverted to main.
nix develop -c forge test --match-contract LibHexStringBytesToHexTest:

[tree: SRC=expect-main TST=expect-clause]
[FAIL: next call did not revert as expected;
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnCharacterAboveDigitRange() (gas: 450874)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnCharacterAboveLetterRange() (gas: 450896)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnCharacterBelowDigitRange() (gas: 450829)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnCharacterBelowLetterRange() (gas: 450852)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnNonHexFirstPayloadCharacter() (gas: 450853)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnNonHexLastPayloadCharacter() (gas: 450851)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnNonHexVmOutput() (gas: 450873)
[FAIL: next call did not revert as expected] testBytesToHexRevertsOnUpperCaseVmOutput() (gas: 450831)
Suite result: FAILED. 31 passed; 9 failed; 0 skipped; finished in 153.54ms (1.08s CPU time)

The [tree: ...] line is the harness asserting, by grep, which version of each of the
two files is on disk before it measures. The nameless ninth [FAIL row is the fuzz
property: forge prints a fuzz test's name after its counterexample, and the harness
strips from counterexample onward to keep the transcript short, which takes the name
with it. Section 2 runs that property on its own, so the ninth failure is not taken on
trust. 31 + 9 = 40, the whole contract, so this is not a filter that matched nothing.

Unmutated baseline for the same command is Suite result: ok. 40 passed; 0 failed.

2. The fuzz oracle fails first on its own, with its charset clause in and the
library still main's, at three seeds from a clean corpus:

nix develop -c forge test --match-test testBytesToHexStripsOrRevertsForEveryVmOutput --fuzz-seed $s:

[tree: SRC=expect-main TST=expect-clause]
--- seed 1
[FAIL: next call did not revert as expected; counterexample: args=[0xa06ba04bbe5e8262d2d038226edd523ef7, "<v", true]] testBytesToHexStripsOrRevertsForEveryVmOutput(bytes,string,bool) (runs: 10, μ: 220424, ~: 197578)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.32ms (4.98ms CPU time)
[tree: SRC=expect-main TST=expect-clause]
--- seed 2
[FAIL: next call did not revert as expected; counterexample: args=[0x00000000000000000000000000000000000000000000000000000000000043d5, "bytes32 constant ", true]] testBytesToHexStripsOrRevertsForEveryVmOutput(bytes,string,bool) (runs: 12, μ: 225967, ~: 219879)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.38ms (5.06ms CPU time)
[tree: SRC=expect-main TST=expect-clause]
--- seed 3
[FAIL: next call did not revert as expected; counterexample: args=[0xaabb, "memory allocated after the call was corrupted", true]] testBytesToHexStripsOrRevertsForEveryVmOutput(bytes,string,bool) (runs: 5, μ: 197548, ~: 197553)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.83ms (5.50ms CPU time)

Each counterexample is a constructed accept-arm case — the third argument is
conforming, true in all three, so the stub Vm returns "0x" followed by a payload
of the right length repeating filler ("<v", "bytes32 constant ", "memory allocated after the call was corrupted"). Right length, right prefix, non-hex payload:
main's library returns where the strengthened oracle now requires a revert. The mirror of this run — oracle charset rule removed, library
fixed — fails the other way at the same three seeds, with
UnexpectedHexString("0x<v<v<v…", 36), UnexpectedHexString("0xbytes32 constant …", 66)
and UnexpectedHexString("0xmemo", 6). That is row M13 of the matrix, run alone.

3. Passing after the fix.nix develop -c forge test (whole suite):

Ran 23 test suites in 2.24s (21.13s CPU time): 164 tests passed, 0 failed, 0 skipped (164 total tests)

main at 89cb0a2, same command, same checkout:

Ran 23 test suites in 3.66s (38.87s CPU time): 155 tests passed, 0 failed, 0 skipped (155 total tests)

The 9 added here are the whole difference.

4. nix develop -c forge fmt --check → exit 0, no diff. git status clean.

5. Coverage.nix develop -c forge coverage --no-match-coverage "test|script":

| File | % Lines | % Statements | % Branches | % Funcs |
| src/lib/LibCodeGen.sol | 100.00% (49/49) | 100.00% (62/62) | 100.00% (3/3) | 100.00% (14/14) |
| src/lib/LibFs.sol | 100.00% (20/20) | 100.00% (17/17) | 100.00% (4/4) | 100.00% (5/5) |
| src/lib/LibHexString.sol | 100.00% (21/21) | 100.00% (25/25) | 100.00% (4/4) | 100.00% (1/1) |
| Total | 100.00% (90/90) | 100.00% (104/104) | 100.00% (11/11) | 100.00% (20/20) |

6. Mutation matrix. Each row breaks exactly one line, re-runs the whole
LibHexStringBytesToHexTest contract, and restores the line. The unmutated
baseline is Suite result: ok. 40 passed; 0 failed, and every row below reports
a suite result over 40 tests, so no row is a filter that matched nothing.

#	file	line broken	what it breaks	suite result	killed by
M1a	src	hexCharset = false; → hexCharset = true;	scan runs but never records a failure	31 passed; 9 failed	all 8 reject tests + the fuzz property
M1b	src	if and(hexCharset, eq(shr(240, mload(add(hexString, 0x20))), 0x3078)) { → if eq(shr(240, mload(add(hexString, 0x20))), 0x3078) {	scan result no longer gates the strip	31 passed; 9 failed	all 8 reject tests + the fuzz property
M2	src	for (uint256 i = 2; i < returned.length; i++) → i = 3	first payload character unscanned	34 passed; 6 failed	…RevertsOnNonHexFirstPayloadCharacter, the four range-boundary tests + the fuzz property
M3	src	same loop → i + 1 < returned.length	last payload character unscanned	38 passed; 2 failed	…RevertsOnNonHexLastPayloadCharacter + the fuzz property
M4	src	c >= 0x30 → c >= 0x2f	accepts /	38 passed; 2 failed	…RevertsOnCharacterBelowDigitRange + the fuzz property
M5	src	c <= 0x39 → c <= 0x3a	accepts :	38 passed; 2 failed	…RevertsOnCharacterAboveDigitRange + the fuzz property
M6	src	c >= 0x61 → c >= 0x60	accepts a backtick	39 passed; 1 failed	…RevertsOnCharacterBelowLetterRange
M7	src	c <= 0x66 → c <= 0x67	accepts g	39 passed; 1 failed	…RevertsOnCharacterAboveLetterRange
M8	src	c >= 0x61 → c >= 0x41	drops the lower bound to A, so everything from A to f passes — upper case A-F, but also G-Z and the six punctuation code points between Z and a	34 passed; 6 failed	…RevertsOnUpperCaseVmOutput, …OnCharacterBelowLetterRange, …OnNonHexVmOutput, …OnNonHexFirstPayloadCharacter, …OnNonHexLastPayloadCharacter + the fuzz property
M9	src	c >= 0x30 → c >= 0x31	over-tightens: rejects a legal 0	25 passed; 15 failed	15 tests, testBytesToHexAcceptsEveryHexNibble among them
M10	src	c <= 0x39 → c <= 0x38	over-tightens: rejects a legal 9	28 passed; 12 failed	12 tests, testBytesToHexAcceptsEveryHexNibble among them
M11	src	c >= 0x61 → c >= 0x62	over-tightens: rejects a legal a	23 passed; 17 failed	17 tests, testBytesToHexAcceptsEveryHexNibble among them
M12	src	c <= 0x66 → c <= 0x65	over-tightens: rejects a legal f	24 passed; 16 failed	16 tests, testBytesToHexAcceptsEveryHexNibble among them
M13	test	for (uint256 i = 2; conforms && i < returned.length; i++) → for (uint256 i = 2; i < 2; i++)	the oracle's charset rule, library left fixed	39 passed; 1 failed	testBytesToHexStripsOrRevertsForEveryVmOutput

src is src/lib/LibHexString.sol, test is
test/src/lib/LibHexString.bytesToHex.t.sol. Passed + failed is 40 on every row.

No mutant survived.

Three things the matrix is arranged to show beyond bare kill counts:

• M4-M7 loosen each range boundary by one ASCII code point, M9-M12 tighten it by
  one. A suite that only asserted some non-hex character is rejected would let
  M4-M7 survive; killing them is what pins the boundaries themselves. M9-M12 are the
  over-tightening rows, and they exist because a guard is a hazard if nothing tests
  that it still accepts every legal input: testBytesToHexAcceptsEveryHexNibble is
  among the killers of all four, which is what makes it worth its place next to eight
  reject tests.
• M1a and M1b separate the scan from the gate. One leaves the loop running and
  discards its verdict, the other leaves the verdict computed and ignores it at the
  and(hexCharset, …). Both are killed by the same nine, so the tests bind the
  scan and the gate together rather than either alone.
• M13 is a mutation of the test file, not the library. Removing the charset rule
  from the fuzz oracle while the library stays fixed has to fail, or the oracle is
  weaker than the code and LibHexString.bytesToHex documents a hex-charset guarantee it does not check, and the fuzz oracle encodes the same gap #68's second half is unaddressed. It fails.

M1a is the one row where the mutation is not a 1-for-1 textual swap: hexCharset = true;
already appears once as the declaration, so the harness logged occurrences before=1 after=2. Every other row is before=1 after=1.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in:5 minutes

Limit details: You’ve used all 1 included review currently available under your plan.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: ebcce2f6-4df9-4070-b46e-aa91a112d432

📥 Commits

Reviewing files that changed from the base of the PR and between 89cb0a2 and eb344aa.

📒 Files selected for processing (2)

• src/lib/LibHexString.sol
• test/src/lib/LibHexString.bytesToHex.t.sol

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}