Conversation
Summary of ChangesHello @spiral-ladder, 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 integrates the 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
The pull request introduces BLST bindings for NAPI, enabling public key and signature operations, along with verification functions. New TypeScript interfaces and test cases have been added to support this functionality. The overall structure for exposing BLST to NAPI is clear. However, there are critical concerns regarding memory management in the bindings/napi/blst.zig file, as it directly violates the repository's strict policy against dynamic memory allocation after initialization. This needs immediate attention to align with the project's design goals for safety and performance.
| var gpa: std.heap.DebugAllocator(.{}) = .init; | ||
| const allocator = if (builtin.mode == .Debug) | ||
| gpa.allocator() | ||
| else | ||
| std.heap.c_allocator; |
There was a problem hiding this comment.
The repository style guide (line 93) explicitly states, "All memory must be statically allocated at startup. No memory may be dynamically allocated (or freed and reallocated) after initialization." Using std.heap.c_allocator in release mode implies dynamic memory allocation, which directly contradicts this critical memory management policy. This needs to be re-evaluated to ensure all memory is managed statically or through pre-allocated pools.
References
- All memory must be statically allocated at startup. No memory may be dynamically allocated (or freed and reallocated) after initialization.
| const pk = try allocator.create(PublicKey); | ||
| errdefer allocator.destroy(pk); |
There was a problem hiding this comment.
The use of allocator.create(PublicKey) constitutes dynamic memory allocation, which is strictly forbidden by the repository's memory management policy (line 93). This is a critical violation that needs to be addressed. Consider using pre-allocated memory or a different approach that aligns with the static allocation requirement.
References
- All memory must be statically allocated at startup. No memory may be dynamically allocated (or freed and reallocated) after initialization.
| const pks = try allocator.alloc(PublicKey, pks_len); | ||
| defer allocator.free(pks); |
There was a problem hiding this comment.
The allocator.alloc(PublicKey, pks_len) call represents dynamic memory allocation, which is explicitly prohibited by the repository's memory management policy (line 93). This is a critical issue that requires a solution using static memory or pre-allocated buffers.
References
- All memory must be statically allocated at startup. No memory may be dynamically allocated (or freed and reallocated) after initialization.
| import { createRequire } from "node:module"; | ||
| import { join } from "node:path"; |
There was a problem hiding this comment.
The removal of the TODO comment is fine, but the change in import formatting from import {createRequire} from "node:module"; to import { createRequire } } from "node:module"; is a stylistic change. While not explicitly covered for TypeScript, consistency in formatting is generally good. Consider running a linter/formatter if available for TypeScript files to maintain a consistent style.
| }); | ||
|
|
||
| it("should throw on invalid length", () => { | ||
| expect(() => bindings.blst.Signature.fromBytes(new Uint8Array(95))).toThrow(); |
There was a problem hiding this comment.
This test uses toThrow() without specifying the expected error message or type. For better test clarity and robustness, it's recommended to assert the specific error that is expected, similar to other tests in this file (e.g., toThrow("InvalidSignatureLength")). This helps ensure that the correct error condition is being met.
| expect(() => bindings.blst.Signature.fromBytes(new Uint8Array(95))).toThrow(); | |
| expect(() => bindings.blst.Signature.fromBytes(new Uint8Array(95))).toThrow("InvalidSignatureLength"); |
| it("should throw on wrong message length", () => { | ||
| const pk = bindings.blst.PublicKey.fromBytes(TEST_VECTORS.publicKey.compressed); | ||
| const sig = bindings.blst.Signature.fromBytes(TEST_VECTORS.signature.compressed); | ||
| expect(() => bindings.blst.fastAggregateVerify(new Uint8Array(31), [pk], sig)).toThrow(); |
There was a problem hiding this comment.
Similar to the previous comment, this test uses toThrow() without specifying the expected error. It would be more robust to assert the specific error message, such as toThrow("InvalidMessageLength"), to ensure the correct error path is being tested.
| expect(() => bindings.blst.fastAggregateVerify(new Uint8Array(31), [pk], sig)).toThrow(); | |
| expect(() => bindings.blst.fastAggregateVerify(new Uint8Array(31), [pk], sig)).toThrow("InvalidMessageLength"); |
5f91d82 to
b9cd9b8
Compare
b9cd9b8 to
4bb7e47
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d54cfcddf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn PublicKey_ctor(env: napi.Env, cb: napi.CallbackInfo(0)) !napi.Value { | ||
| const pk = try allocator.create(PublicKey); | ||
| errdefer allocator.destroy(pk); | ||
| _ = try env.wrap(cb.this(), PublicKey, pk, PublicKey_finalize, null); |
There was a problem hiding this comment.
Initialize PublicKey in constructor or make it unusable
The constructor allocates a PublicKey but leaves it uninitialized, and the class is publicly exposed in JS/TS. Calling new blst.PublicKey() (or any code path that creates an instance without fromBytes) yields garbage memory; subsequent toBytes() or verify() calls will operate on undefined data and can return nondeterministic results. This is easy to trigger because the TypeScript declaration does not hide the constructor. Consider initializing to a known value or throwing in the constructor so only fromBytes can create valid instances.
Useful? React with 👍 / 👎.
| pub fn Signature_ctor(env: napi.Env, cb: napi.CallbackInfo(0)) !napi.Value { | ||
| const sig = try allocator.create(Signature); | ||
| errdefer allocator.destroy(sig); | ||
| _ = try env.wrap(cb.this(), Signature, sig, Signature_finalize, null); |
There was a problem hiding this comment.
Initialize Signature in constructor or make it unusable
Like PublicKey_ctor, Signature_ctor allocates a Signature without initialization while exposing a public JS constructor. new blst.Signature() therefore produces an object backed by uninitialized memory; calling toBytes*() or passing it into verify() will read garbage bytes and can lead to incorrect verification results. Consider zero‑initializing or throwing in the constructor so instances must be created via fromBytes.
Useful? React with 👍 / 👎.
Co-authored-by: Cayman <caymannava@gmail.com>
No description provided.