Correctness, diagnostics & bench/coverage hardening (Zig-book adoption)#102
Merged
Conversation
Adopts idioms surfaced by cross-reading "Systems Programming with Zig" against the SDK. Four contained fixes: 1. uint256 big-endian conversion is now host-endianness independent. toBigEndianBytes/fromBigEndianBytes used @byteswap, correct only on little-endian hosts; on a big-endian target every ABI word, event topic and tx field would be byte-reversed. Switch to std.mem.writeInt /readInt with explicit .big (same codegen on LE, correct on BE). 2. ContractAbi (abi_json) is now backed by a single ArenaAllocator. The old per-allocation cleanup leaked already-built entries when a later ABI item failed to parse (and on a toOwnedSlice OOM). The arena makes the error path leak-free and deletes the recursive freeParams. Regression test added (leaks would trip std.testing.allocator). 3. Provider now surfaces JSON-RPC error diagnostics. Scalar calls threw an opaque error.RpcError, discarding code/message; only the batch path kept them. Provider.lastError() now exposes {code, message} so callers can tell an on-chain revert (code 3) from a transport failure. 4. Provider.next_id is now std.atomic.Value(u64) (fetchAdd), so a shared Provider cannot hand out duplicate request ids.
…infra)
- bench harness now reports per-batch stddev alongside ns/op, and emits a
machine-readable `BENCH_JSON|{...}` line per benchmark (name/ns_per_op/
stddev_ns/iters). compare.sh filters those lines from its human view.
- Add a nightly + on-demand Bench workflow that runs the harness off the
PR-blocking path and uploads results (incl. the JSONL) as an artifact.
- Coverage: `zig build install-test` emits the unit-test binary; `make
coverage` runs kcov over src/ (excluding vendored crypto); a non-required
CI job publishes the kcov report as an artifact.
- New build steps: `zig build fmt-check` (mirrors the CI fmt job) and
`zig build docs` (emits API docs to zig-out/docs). Makefile gains
`coverage`/`docs` targets; .gitignore ignores kcov-out/.
|
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 Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds per-call RPC error diagnostics and atomic request IDs to ChangesSource Library Changes
Tooling, CI, and Benchmark Improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/bench.yml (1)
12-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden workflow token scope and pin action revisions.
Line [12] lacks explicit
permissions, and Lines [18], [19], [27] use mutable tags instead of immutable SHAs. Pinning and least-privilege permissions are needed to reduce supply-chain and token-risk exposure.Suggested hardening
+permissions: + contents: read + jobs: bench: @@ - - uses: actions/checkout@v4 - - uses: mlugg/setup-zig@v2 + - uses: actions/checkout@<pinned-commit-sha> + with: + persist-credentials: false + - uses: mlugg/setup-zig@<pinned-commit-sha> @@ - - name: Upload benchmark results - uses: actions/upload-artifact@v4 + - name: Upload benchmark results + uses: actions/upload-artifact@<pinned-commit-sha>🤖 Prompt for 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. In @.github/workflows/bench.yml around lines 12 - 33, The workflow lacks explicit permissions configuration and uses mutable version tags for actions instead of immutable commit SHAs. Add a permissions block at the job level (after the runs-on line in the bench job) with minimal required permissions, and pin all action revisions by replacing the mutable tags: update actions/checkout@v4 to its specific commit SHA, mlugg/setup-zig@v2 to its specific commit SHA, and actions/upload-artifact@v4 to its specific commit SHA. This reduces supply-chain risk and token exposure by using immutable references and following least-privilege principles.Source: Linters/SAST tools
🤖 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 @.github/workflows/ci.yml:
- Around line 51-71: The coverage job needs GitHub Actions security hardening
applied. Add a permissions block at the job level in the coverage job (after the
runs-on line) to specify explicit least-privilege token permissions.
Additionally, pin the three action references to immutable commit SHAs instead
of mutable version tags: replace actions/checkout@v4, mlugg/setup-zig@v2, and
actions/upload-artifact@v4 with their full commit SHA values to prevent
supply-chain drift from tag mutations.
In `@bench/compare.sh`:
- Line 22: The grep -v command on line 22 can return exit code 1 when all lines
matching the pattern are filtered out, causing the script to abort under
pipefail mode even when zig build bench succeeds. Fix this by adding a fallback
to true after the grep -v pipeline, such that the overall pipeline always
returns 0 regardless of whether grep finds matching lines or not. This allows
the filtered output to be echoed without aborting the script due to a non-zero
exit code from grep.
In `@src/abi_json.zig`:
- Around line 130-142: The parseInputs function is too lenient when handling
malformed input, treating invalid data as empty parameter lists instead of
rejecting it. On line 134, when a field exists but is not an array, change the
return statement from returning an empty slice to returning an error from the
ComponentError union (such as UnknownType). Similarly, on line 138, change the
non-object item handling from silently continuing the loop to returning an
error, so that malformed entries in the array are properly rejected rather than
ignored. This ensures that invalid ABIs cannot be parsed into different
signatures or shapes.
- Around line 31-33: The catch block in the parseFromSliceLeaky call currently
masks all errors as error.InvalidJson, which prevents callers from
distinguishing between allocation failures and malformed input. Modify the catch
block to explicitly check if the caught error is error.OutOfMemory and propagate
it directly, while converting all other parsing errors to error.InvalidJson.
This allows the caller to handle resource exhaustion separately from invalid
JSON input.
In `@src/provider.zig`:
- Line 23: Update the integration tests in tests/integration_tests.zig to
properly access the atomic next_id field from the provider object. Since next_id
is now defined as std.atomic.Value(u64) in src/provider.zig, all direct accesses
to provider.next_id at lines 385, 388, 390, and 392 must be wrapped with the
.load() method call to retrieve the actual u64 value. Use an appropriate memory
ordering parameter like .acquire or .seq_cst in the .load() calls depending on
your synchronization requirements, such as provider.next_id.load(.acquire).
---
Outside diff comments:
In @.github/workflows/bench.yml:
- Around line 12-33: The workflow lacks explicit permissions configuration and
uses mutable version tags for actions instead of immutable commit SHAs. Add a
permissions block at the job level (after the runs-on line in the bench job)
with minimal required permissions, and pin all action revisions by replacing the
mutable tags: update actions/checkout@v4 to its specific commit SHA,
mlugg/setup-zig@v2 to its specific commit SHA, and actions/upload-artifact@v4 to
its specific commit SHA. This reduces supply-chain risk and token exposure by
using immutable references and following least-privilege principles.
🪄 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 Plus
Run ID: 461356a4-e93f-4dc6-b37f-bf30c6b1c398
📒 Files selected for processing (10)
.github/workflows/bench.yml.github/workflows/ci.yml.gitignoreMakefilebench/bench.zigbench/compare.shbuild.zigsrc/abi_json.zigsrc/provider.zigsrc/uint256.zig
- Add `permissions: contents: read` to the CI and Bench workflows (resolves CodeQL actions/missing-workflow-permissions). Artifact upload uses the Actions runtime token, so no extra scope is needed. - Pin actions/checkout, mlugg/setup-zig and actions/upload-artifact to commit SHAs (resolves CodeQL actions/unpinned-tag). - Coverage job: kcov was dropped from Ubuntu's repos (gone in 24.04) and ships no prebuilt binaries, so the `apt-get install kcov` step failed. Build a pinned kcov release (v43) from source instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/bench.yml:
- Line 22: The checkout action at the beginning of the bench workflow does not
include the persist-credentials parameter, which means the GitHub token will
remain accessible to subsequent build steps. Add persist-credentials: false as a
parameter to the actions/checkout step to prevent the token from being
accessible to build scripts or potentially included in uploaded artifacts,
following the security best practice of limiting credential exposure.
In @.github/workflows/ci.yml:
- Line 24: Add the `persist-credentials: false` parameter to all four
`actions/checkout` steps in the workflow file at lines 24, 35, 49, and 62 to
prevent the GITHUB_TOKEN from being exposed to subsequent steps. For each
`actions/checkout` action, add a new line with the parameter
`persist-credentials: false` under the `with:` section (or create a `with:`
section if it doesn't exist) to explicitly disable credential persistence as a
defense-in-depth security measure alongside the existing workflow-level
`contents: read` permission.
🪄 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 Plus
Run ID: ad86cdae-e30d-47c9-813a-c899b43b6d54
📒 Files selected for processing (2)
.github/workflows/bench.yml.github/workflows/ci.yml
- tests/integration_tests.zig: access the now-atomic provider.next_id via .load(.monotonic) (lines 385/388/390/392). These didn't compile after the atomic change; the unit-test CI job never builds the integration suite, so it slipped through. Verified `zig build integration-test` now compiles. - abi_json fromJson: propagate error.OutOfMemory from parseFromSliceLeaky instead of masking allocator exhaustion as error.InvalidJson. - bench/compare.sh: `grep -v` can exit 1 if it filters every line, aborting the script under `set -o pipefail`; append `|| true`. - ci.yml/bench.yml: add `persist-credentials: false` to all checkout steps (defense in depth; keeps the token out of later steps/artifacts). Skipped (with reason): the lenient parseInputs handling (non-array field -> empty list, non-object entries skipped) is pre-existing behavior preserved verbatim by the arena refactor; tightening ABI-validation strictness is a separate behavioral change out of scope for this PR.
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
2 participants
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.
What
Adopts practices surfaced by cross-reading Systems Programming with Zig (mactsouk/zigSP, GPL-3.0) against this MIT codebase. Idioms only — no code copied. Most of the book is confirmatory: eth.zig already does comptime selectors, lock-free nonce management, leak-checked tests, and a criterion-style bench with a Rust-
alloycomparison better than the book. This PR lands the genuine gaps.All changes validated locally on Zig 0.16.0:
zig build,zig build fmt-check,zig fmt --check src/ tests/, andzig build testall pass.Tier 1 — correctness & diagnostics (
55e9a0f)u256 big-endian conversion is now host-endianness independent.
uint256.toBigEndianBytes/fromBigEndianBytesused@byteSwap/@bitCast, correct only on little-endian hosts; on a big-endian target every ABI word, event topic, and tx field would be byte-reversed. Switched tostd.mem.writeInt/readIntwith explicit.big(identical codegen on LE, correct on BE). Strengthened the known-value test to pin multi-byte byte order. (Latent: no big-endian targets today.)ContractAbi(abi_json) is now backed by a singleArenaAllocator. The old per-allocation cleanup leaked already-built entries when a later ABI item failed to parse (and on atoOwnedSliceOOM). The arena makes the error path leak-free and deletes the recursivefreeParams. Added a regression test — leaks tripstd.testing.allocator.Provider now surfaces JSON-RPC error diagnostics. Scalar calls threw an opaque
error.RpcError, discarding code/message; only the batch path kept them.Provider.lastError()now returns{code, message}so callers can tell an on-chain revert (code 3) from a transport failure. Stored inline (no extra heap, no new deinit).Provider.next_idis nowstd.atomic.Value(u64)(fetchAdd), so a shared Provider can't hand out duplicate request ids.Tier 2 — bench/coverage/build infra (
21cf88f)BENCH_JSON|{name,ns_per_op,stddev_ns,iters}line per benchmark (verified locally).compare.shfilters those from its human view.Benchworkflow (schedule +workflow_dispatch) runs the harness off the PR-blocking path and uploads results (incl. the JSONL) as an artifact — the basis for a future committed baseline + regression diff.zig build install-testemits the unit-test binary;make coveragerunskcovoversrc/(excluding vendored crypto); a non-required CI job publishes the kcov report as an artifact. (kcov is Linux-only; validated via the build step locally, the kcov run is exercised in CI.)zig build fmt-check(mirrors the CI fmt job) andzig build docs(emits API docs tozig-out/docs). Makefile gainscoverage/docs;.gitignoreignoreskcov-out/.Intentionally not included (2 of the 9 assessed findings)
After implementing the rest, two assessed items proved counterproductive as specified, so they were deliberately left out (discussed and agreed):
abi_encode/abi_decode/wallet) already use named error sets (EncodeError/DecodeError/WalletError). The only inferred sets left are on functions wrappingstd.http; naming those would drag the entirestd.httperror surface into the public API, making it less stable. Keeping inferred sets there is the correct design.🤖 Generated with Claude Code
Summary by CodeRabbit