Make unit tests actually execute (and fix the bugs it surfaced)#84
Merged
Conversation
The test step aggregated module tests via `_ = eth.module` field access, which only forces semantic analysis -- it never collects the per-module `test` blocks. zig build test compiled assertions but ran zero of them (a deliberately injected `expect(1 == 2)` passed). CI has been green-on-compile, not green-on-pass, the whole time. Root the test artifact at src/root.zig, whose test block direct-imports every module file (the form that does collect tests), re-attaching the XKCP/secp256k1 C backends. Delete the dead tests/unit_tests.zig. Real defects this surfaced, now fixed: - abi_json: still used the 0.15 ArrayList(...).init API -> migrated to the 0.16 unmanaged API (fromJson was broken on 0.16) - rlp.bytesToUint: `<< 8` failed to compile for small int types -> comptime-guarded the big-int slow path - mnemonic.entropyToMnemonic: comptime-value resolution + u8 overflow (32*8=256) for 24-word phrases -> widen + comptime - abi_json parseInputs/parseParam: inferred-error-set dependency loop -> explicit ComponentError - rlp struct reflection: Type.Struct.fields changed in 0.17-dev -> std.meta.fieldNames (portable across 0.16 and 0.17-dev master) - stale test expectations: formatHash/log-address length typos, a static-tuple ABI length (static tuples encode inline = 64 not 96), fixed-size hex length error mapping - vendored XKCP/secp256k1 C built with -fno-sanitize=undefined so their intentional unaligned u64 loads don't trap Debug UBSan (38 crashes) Quarantined 4 pre-existing correctness bugs as visible skips with tracking issues: broken pure-Zig Keccak fallback (#80), mulDiv u256 boundary (#81), BIP-39 passphrase seed vector (#82), DEX round-trip assumption (#83). Result: 749 pass, 16 skip on 0.16.0; passes on 0.17-dev master.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReroot unit-test artifact at src/root.zig and reattach vendored C backends with UBSan undefined instrumentation disabled; apply allocator/error-set fixes to ABI JSON parsing, switch RLP struct reflection and add wide-integer decoding support, correct test expectations, and quarantine several flaky/failing tests. ChangesUnit test infrastructure rework and defect remediation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/rlp.zig`:
- Around line 85-90: The .@"struct" branch of encodeInto still iterates using
s.fields; update that loop to use std.meta.fieldNames(`@TypeOf`(value)) like the
other struct path so reflection works on Zig 0.16/0.17. Replace occurrences of
inline for (s.fields) { ... `@field`(value, field_name) ... } with inline for
(comptime std.meta.fieldNames(`@TypeOf`(value))) |field_name| { ... } and ensure
any uses of field lookup or length computation (e.g., encodedLength/@field
calls) use the new field_name variable from std.meta.fieldNames.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f27aa4b-7e06-4a05-9a98-72f16cae1c09
📒 Files selected for processing (13)
CHANGELOG.mdbuild.zigsrc/abi_encode.zigsrc/abi_json.zigsrc/dex/v2.zigsrc/hex.zigsrc/keccak_optimized.zigsrc/mev_share.zigsrc/mnemonic.zigsrc/rlp.zigsrc/subscription.zigsrc/uint256.zigtests/unit_tests.zig
💤 Files with no reviewable changes (1)
- tests/unit_tests.zig
encodeInto's struct branch still used the pre-0.17 Type.Struct.fields layout; CI missed it because no running test instantiates that path with a struct. Use std.meta.fieldNames like the other two sites for 0.16/0.17-dev portability.
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
NonceManager seeds lazily (no RPC in init) from the pending tx count, then hands out strictly increasing nonces via a lock-free atomic fetch-and-add. Seeding uses double-checked locking so exactly one thread performs the RPC; the hot path (next after seeding) takes no lock. onFailure(nonce) rolls the counter back only when nonce is the high-water mark (CAS), a documented no-op otherwise since a lower nonce cannot be safely reused once a higher one is in flight. Optional Wallet integration via a nullable nonce_manager field; existing behavior unchanged when null. Uses std.Io.Mutex (Zig 0.16 moved concurrency under std.Io) via the default runtime io. New docs page nonce-manager.mdx. 10 unit tests covering sequencing, onFailure semantics, peek, and an 8-thread concurrency check -- these now actually execute under the revived test harness (#84), which caught two compile errors in the original draft (std.Thread.Mutex and refAllDeclsRecursive, both gone in 0.16).
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
The lane-complementing chi optimization (from #22) was wrong: it kept lanes {1,2,8,12,17,20} complemented across rounds but the chiLane operation-selection did not correctly maintain that invariant, so keccak_optimized.keccak256 produced incorrect digests for all inputs. This was a pure-Zig fallback never wired into keccak.zig (which uses XKCP), so it went undetected until the test harness was fixed (#84). Replace the entire lane-complement scheme with the standard chi step a[i] = b[i] ^ (~b[i+1] & b[i+2]). Modern LLVM lowers ~a & b to a single ANDN, so there is no meaningful speed loss on the (non-default) fallback path, and the code is now obviously correct. All 13 tests pass: pinned keccak256 vectors plus cross-validation against std.crypto across sizes through 64KB.
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
…87) mulDiv(MAX, MAX, MAX) should equal MAX but returned a slightly smaller value. The overflow path used a limb-native Knuth Algorithm D divider (divWide) that mis-handled a full multi-limb divisor. Isolated: mulDiv(MAX, 2, 2) was correct (small divisor) but the all-limbs-set divisor case was not. Replace the overflow path's mulWide+divWide with native u512: product = a*b in u512, quotient = product/denominator, null if it exceeds u256. Simple and provably correct. The fast u128 path (the benchmarked 18ns hot path) and the medium u256 path are unchanged -- only the rare a*b-overflows-u256 branch is affected. Removed the now dead, buggy divWide. Surfaced by the test-harness fix (#84).
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
mnemonic.toSeed was correct all along -- it reproduces the canonical Trezor BIP-39 seed for all-zero entropy + "TREZOR": c55257c360c07c72029aebc1b53c05ed0362ada38ead3e3e9efa3708e53495531f09a6987599d18264c1e1c92f2cf141630c7a3c4ab7c81b2f001698e7463b04 (byte-identical to a direct std.crypto.pwhash.pbkdf2 call). The quarantined test's expected_first32/last32 were simply wrong (the second half of first32 read 7e24052f0b7c87c2 instead of the real 9efa3708e5349553). Fixed the expected values and un-quarantined. So #82 was a bad test, not a code bug. Surfaced by the test-harness fix (#84) like the rest.
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
#89) * Fix getAmountIn round-trip test to use realistic reserves (closes #83) The getAmountIn/getAmountOut implementations are correct standard Uniswap V2. The quarantined round-trip test used reserve_out=200e9 (tiny vs reserve_in=100e18): getAmountOut floors a small output, discarding ~4e8 wei of input-equivalent, which the +1-unit ceiling in getAmountIn cannot recover -- so recovered_input legitimately falls ~4e8 below amount_in. That is correct AMM behaviour; the test's within-2-units assumption was wrong for those parameters. Use balanced reserves (reserve_out=200e18) where the floored output is large; the round-trip is then exact (diff 0). Documented why the invariant only holds for adequately-large outputs. #83 was a bad test, not a code bug -- the last of the four issues the test-harness fix (#84) surfaced. The unit suite now runs with zero skips (775/775). * Address review: reconcile changelog quarantine note (all four resolved)
Merged
koko1123
added a commit
that referenced
this pull request
Jun 10, 2026
Consolidate the Unreleased section into a clean 0.6.0 entry: - Added: nonce_manager (#75), mev_share (#34), eth_sendPrivateTransaction (#40), MEV-Share backrunner example (#38) - Changed: benchmark refresh vs alloy 1.6/2.0 (18/26) - Fixed: the test-harness fix (#84) plus the four issues it surfaced (#80-83) and the sse_transport feedLine break Align versions: build.zig.zon 0.5.0 -> 0.6.0, README/docs install URLs, SECURITY.md supported versions -> 0.6.x.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
zig build testhas never run a single unit-test assertion. The test step aggregated module tests via_ = eth.modulefield access, which only forces semantic analysis -- it does not collect the per-moduletestblocks into the test binary. Proof: injectingtest { try expect(1 == 2); }intosrc/hex.zigstill exits 0.So CI has been green-on-compile, not green-on-pass. The 0.16 migration, @Splat work, etc. were validated only insofar as they compiled.
The fix
Root the test artifact at
src/root.zig, whose test block direct-imports every module file (_ = @import("hex.zig")-- the form that does collect tests), re-attaching the XKCP/secp256k1 C backends. Delete the now-deadtests/unit_tests.zig.Turning the suite on revealed 58 problems (20 assertion failures + 38 Debug-only crashes). Breakdown:
Real defects fixed
ArrayList(...).initAPI --ContractAbi.fromJsonwas broken on 0.16, only reachable via its never-run test. Migrated to the unmanaged API.<< 8didn't compile for small int instantiations -> comptime-guarded the >64-bit slow path.entropy_bytes * 8u8 overflow (32*8=256) for 24-word phrases.ComponentError.Type.Struct.fieldswas restructured in 0.17-dev ->std.meta.fieldNames(portable across 0.16 and master).formatHash/log-address hex-length typos, a static-tuple ABI length (static tuples encode inline = 64, not 96), fixed-size hex length-error mapping.-fno-sanitize=undefined-- their intentional unaligned u64 loads were tripping the Debug UBSan runtime (the issue reverted in d369bd0). ReleaseFast/production was never affected.Quarantined (visible skips + tracking issues, not silent passes)
Four genuine pre-existing correctness bugs, marked
error.SkipZigTest:mulDiv(MAX, MAX, MAX)wrong at the u256 boundaryResult
zig fmtcleanThis is the highest-leverage correctness fix in the repo: every future test will now actually run.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Chores