test(utils): add unit tests for domain computation utilities#299
test(utils): add unit tests for domain computation utilities#299spiral-ladder merged 7 commits intoChainSafe:mainfrom
Conversation
Add 8 unit tests for domain.zig covering: - computeDomain: domain type in first 4 bytes, different types/versions produce different domains, fork data root portion is non-zero - forkVersion: returns previous version before fork epoch, current at/after - computeForkDataRoot: deterministic output, different inputs produce different roots 🤖 Generated with AI assistance
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a suite of unit tests for the domain computation utilities, which are critical components for BLS signature verification. By establishing these tests, the codebase now has baseline coverage for these functions, ensuring that domain structure and fork versioning logic adhere to the consensus specifications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive suite of unit tests for domain and fork utility functions in src/state_transition/utils/domain.zig, covering domain computation, fork versioning, and fork data root generation. The review feedback identifies an opportunity to simplify a manual loop in the computeDomain test by using std.mem.all, which would improve code readability and better align with the project's style guidelines regarding simplicity and developer experience.
| // Remaining 28 bytes should be from fork data root (non-zero due to hashing) | ||
| var all_zero = true; | ||
| for (domain[4..32]) |b| { | ||
| if (b != 0) { all_zero = false; break; } | ||
| } |
There was a problem hiding this comment.
The manual loop to check if the remaining bytes are non-zero can be simplified using std.mem.all. This improves readability and conciseness, aligning with the repository's style guide emphasis on simpler designs and good developer experience.
try testing.expect(!std.mem.all(u8, domain[4..32], 0));
References
- Good style advances safety, performance, and developer experience. Declare variables at the smallest possible scope, and minimize the number of variables in scope. Don't introduce variables before they are needed. (link)
Fork SSZ container uses snake_case field names (previous_version, current_version), not camelCase. Also fix zig fmt formatting. 🤖 Generated with AI assistance
no need to test that computeForkDataRoot with the exact same input produces same root
There was a problem hiding this comment.
made some opinionated refactors (eg. removing what i think are redundant tests) mainly aimed at reducing slop tests that bloat LoCs, cc @GrapeBaBa, can retroactively update this if you think there's some glaring mistakes
3 participants
Summary
Add 8 unit tests for
domain.zig— the domain computation utilities used in BLS signature verification.Tests added
Motivation
domain.zighad 0 test coverage despite being critical for signature verification correctness. These functions compute the BLS signing domain per the consensus spec.🤖 Generated with AI assistance