fix: export asyncAggregateWithRandomness through napi binding - #371
Conversation
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 addresses an issue where Highlights
New Features🧠 You can now enable Memory (public preview) 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 the 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 counterproductive. 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
|
There was a problem hiding this comment.
Code Review
This pull request implements the asyncAggregateWithRandomness function within the Zig NAPI bindings, refactoring the implementation to use js.Array and js.convertReturn while updating internal structures to use native public key and signature types. Comprehensive TypeScript tests were also added to verify the new asynchronous functionality. Review feedback identifies several violations of the repository style guide, specifically regarding the use of abbreviated variable names like stat, the use of underscores in function names, and the requirement to include assertions for all function arguments to maintain the necessary assertion density.
| fn asyncAggRand_complete(env: napi.Env, stat: napi.status.Status, data: *AsyncAggRandData) void { | ||
| defer { |
There was a problem hiding this comment.
The variable name stat is an abbreviation of status, which violates the repository style guide (Rule 221). Additionally, the function name asyncAggRand_complete uses an underscore, violating the camelCase requirement for function names (Rule 216). Finally, per Rule 51, function arguments should be asserted to ensure they are valid before use.
fn asyncAggRand_complete(env: napi.Env, status: napi.status.Status, data: *AsyncAggRandData) void {
std.debug.assert(env.env != null);
std.debug.assert(data.work != null);
defer {
| fn settle(env: napi.Env, stat: napi.status.Status, data: *AsyncAggRandData) !void { | ||
| if (stat != .ok) { |
There was a problem hiding this comment.
The variable name stat is an abbreviation of status (Rule 221). Please also add assertions for the function arguments to comply with the repository's assertion density requirements (Rule 51).
fn settle(env: napi.Env, status: napi.status.Status, data: *AsyncAggRandData) !void {
std.debug.assert(env.env != null);
std.debug.assert(data.pks.ptr != null);
if (status != .ok) {
| pub fn asyncAggregateWithRandomness(sets: js.Array) !js.Value { | ||
| const n = try sets.length(); |
There was a problem hiding this comment.
Per Rule 51 of the repository style guide, all function arguments must be asserted. Please add an assertion for the sets argument.
pub fn asyncAggregateWithRandomness(sets: js.Array) !js.Value {
std.debug.assert(sets.val != null);
const n = try sets.length();
References
- Assert all function arguments and return values. (link)
spiral-ladder
left a comment
There was a problem hiding this comment.
LGTM, gemini reviews look pretty pedantic to me
will contain #371 for consumption by ChainSafe/lodestar#8900
* feat: warn when dsl incompatible functions are skipped Zig functions are skipped if they: 1) are not `pub fn`, 2) do not use DSL-compatible types from the `js` module We can't error since we want flexibility to define and use other functions in the same file, so we just add a simple warning to warn the user about this at comptime. This is to avoid future situations encountered here: ChainSafe/lodestar-z#371 * fix: use the compile time error --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com>
will contain ChainSafe#371 for consumption by ChainSafe/lodestar#8900
🤖 I have created a release *beep* *boop* --- ## [1.0.0](v0.1.2...v1.0.0) (2026-08-19) ### Features * add `state.getBuildersLength()` binding ([#472](#472)) ([be2b5ab](be2b5ab)) * **beacon-node:** add block state cache and checkpoint datastore ([#452](#452)) ([2145faa](2145faa)) * bindings to `getExpectedWithdrawals` and native tweaks ([#350](#350)) ([f47bc66](f47bc66)) * **bindings:** add pubkey cache syncPubkeys ([#537](#537)) ([542779f](542779f)) * **bindings:** aggregate cached public keys by validator index ([#397](#397)) ([2f90603](2f90603)) * **bindings:** align `BeaconStateView` with `IBeaconStateView` ([#347](#347)) ([b8ec273](b8ec273)) * **bindings:** configurable pubkey cache growth step ([#481](#481)) ([133ef24](133ef24)) * **bindings:** expose more APIs for STF ([#444](#444)) ([7fe2609](7fe2609)) * **bls:** add small MSM for npoints < 32 ([#393](#393)) ([b430638](b430638)) * **blst:** use external buffers for blst operations ([#358](#358)) ([78e4678](78e4678)) * **ci:** conditionally publish bindings with tag ([#355](#355)) ([ea77919](ea77919)) * **clock:** add clock module for slot/epoch timing ([#354](#354)) ([385b077](385b077)) * **fork_choice:** add Prometheus metrics module ([#309](#309)) ([cbc9d8d](cbc9d8d)) * **forkchoice:** implement the forkchoice module ([#246](#246)) ([7c62a9b](7c62a9b)) * getSyncCommitteesWitness ([#367](#367)) ([ef77649](ef77649)) * implement `loadState` API and binding ([#165](#165)) ([f903519](f903519)), closes [#159](#159) * **metrics:** metrics bindings ([#455](#455)) ([dd41999](dd41999)) * migrate blst,pubkeys to use zapi js dsl ([#331](#331)) ([fcd26ca](fcd26ca)) * **pubkeys:** add getPubkeyBytes binding ([#555](#555)) ([4ca51cf](4ca51cf)) * publish ARM64 musl bindings ([#482](#482)) ([ac764c9](ac764c9)) * **shuffle:** add swap-or-not shuffling module and binding ([#559](#559)) ([c2db37c](c2db37c)) * split nextValue fn ([#464](#464)) ([b47faeb](b47faeb)) * support getLatestWeakSubjectivityCheckpointEpoch ([#366](#366)) ([dcf3883](dcf3883)) * update fulu deposit processing ([#442](#442)) ([064335c](064335c)) ### Bug Fixes * avoid set ([#484](#484)) ([2e25d97](2e25d97)) * better generation of rand scalar ([#388](#388)) ([74dce77](74dce77)) * **bindings:** accept `dontTransferCache` in processSlots for backward compatibility ([#460](#460)) ([65df5af](65df5af)) * **bindings:** check signature infinity by default ([#509](#509)) ([2f5f281](2f5f281)) * **bindings:** clean up failed async BLS work ([#527](#527)) ([1111b00](1111b00)) * **bindings:** free metrics writer on scrape failure ([#529](#529)) ([4c8d94a](4c8d94a)) * **bindings:** harden random aggregate scalars ([#528](#528)) ([8e89a63](8e89a63)) * **bindings:** log level for missing fields ([#435](#435)) ([08faf41](08faf41)) * **bindings:** misordering of print for cpu count ([#381](#381)) ([752a972](752a972)) * **bindings:** populate epoch participation for test fixtures ([#436](#436)) ([8dbdd2e](8dbdd2e)) * **bindings:** refcount Pool to fix teardown panic ([#352](#352)) ([23b2f68](23b2f68)) * **bindings:** roll back partial N-API initialization ([#491](#491)) ([31c5ebb](31c5ebb)) * **bindings:** size BLS thread pool by cgroup-aware CPU count ([#386](#386)) ([3ae9522](3ae9522)) * **bindings:** validate class types before unwrap ([#514](#514)) ([2fd2ad5](2fd2ad5)) * **bindings:** validate secret key hex length ([#517](#517)) ([136e415](136e415)) * **bls:** align PublicKey.uncompress validation with Signature.uncompress ([#508](#508)) ([5a8dbe9](5a8dbe9)) * **bls:** bound randomized aggregation inputs ([#548](#548)) ([779d0bf](779d0bf)), closes [#542](#542) * **bls:** clean up partial thread pool initialization ([#490](#490)) ([d55e598](d55e598)) * **bls:** convert pippenger scratch bytes to element counts ([#513](#513)) ([a12ca92](a12ca92)) * **bls:** enforce 32-byte signing roots ([#545](#545)) ([72fd308](72fd308)) * **bls:** make batch cardinality structural ([#547](#547)) ([a06d8b2](a06d8b2)) * **bls:** preserve aggregate outputs on failure ([#521](#521)) ([e0b6dd1](e0b6dd1)) * **bls:** reject empty keygen salts ([#524](#524)) ([d2a9c86](d2a9c86)) * **bls:** reject unknown BLST error codes ([#525](#525)) ([9e4a6ad](9e4a6ad)) * **bls:** size pairing buffers for 32-bit targets ([#531](#531)) ([dc64a27](dc64a27)) * **blst:** default signature infinity check to true if not provided ([#387](#387)) ([021cdcb](021cdcb)) * **build:** remove `zig-out` from `files` ([#360](#360)) ([c52af09](c52af09)) * **ci:** fix caching spec test version ([#439](#439)) ([96885a1](96885a1)) * dangling state pointer in loadOtherState ([#450](#450)) ([81cbd5f](81cbd5f)) * **epoch_cache:** compute missing `next_proposers` ([#447](#447)) ([0088a29](0088a29)) * **epoch_cache:** populate decision roots in afterProcessEpoch ([#453](#453)) ([4b70a5e](4b70a5e)) * export asyncAggregateWithRandomness through napi binding ([#371](#371)) ([1d04c2b](1d04c2b)) * harden memory safety across PMT, SSZ tree views, and state transition ([#377](#377)) ([d6f5897](d6f5897)) * improve atomic ordering in ThreadPool and NAPI init ([#310](#310)) ([4b0a1cc](4b0a1cc)) * interface compatbility with NativeBeaconStateView ([#445](#445)) ([89e13d1](89e13d1)) * missing deinits in loadOtherState ([#459](#459)) ([094d278](094d278)) * missing state commits ([#454](#454)) ([a432b55](a432b55)) * no-op when syncPubkeys run on a pk cache with shrinking validator set ([#432](#432)) ([ed05a99](ed05a99)) * param order in BeaconBlockBody ([#348](#348)) ([d8b9c06](d8b9c06)) * pendingConsolidations bindings ([#449](#449)) ([b9c497e](b9c497e)) * **pmt,ssz:** harden chunked-leaf and zero-copy tree-view memory safety ([#400](#400)) ([de50c53](de50c53)) * populate cache balances during rewards/penalties processing ([#474](#474)) ([5bf23dc](5bf23dc)) * re-expose sizes ([#369](#369)) ([64b81f3](64b81f3)) * remove `slashValidator` gating on active status ([#448](#448)) ([d319a0d](d319a0d)) * **ssz:** drop redundant default-init pass in fixed-list decode ([#468](#468)) ([0c757be](0c757be)) * **ssz:** publish child cache entries after lookup ([#565](#565)) ([21e78c9](21e78c9)) * state transition binding exports ([#456](#456)) ([895982c](895982c)) * **state-transition:** group-check signature sets ([#515](#515)) ([42774e9](42774e9)), closes [#502](#502) * **state-transition:** isolate epoch step cache mutations ([#535](#535)) ([a83741a](a83741a)) * **state-transition:** repair Pool.init call broken by [#346](https://github.com/ChainSafe/lodestar-z/issues/346)×[#367](https://github.com/ChainSafe/lodestar-z/issues/367) merge skew ([#394](#394)) ([b42944f](b42944f)) * various fixes around config ([#433](#433)) ([c4f082c](c4f082c)) ### Performance Improvements * **bindings:** drop TS BLS comparison benches and report benchmarks on PRs ([#552](#552)) ([c909c6f](c909c6f)) * **bls:** add cache-aware signature verifier ([#562](#562)) ([063857e](063857e)) * **bls:** bypass worker queue for small batches ([#553](#553)) ([3f8a6df](3f8a6df)) * **epoch:** replace AutoHashMap with array lookup in reward/penalty caches ([#286](#286)) ([e4e181b](e4e181b)), closes [#243](#243) * **pmt:** chunked-leaf packing for basic lists and container_struct ([#346](#346)) ([ba156c4](ba156c4)) ### Code Refactoring * allocate `AsyncAggRandData` in one obj ([#384](#384)) ([459750f](459750f)) * **bindings/pubkeys:** simplify allocation strategy for aggregate ([#518](#518)) ([b82750f](b82750f)) * **bindings:** rename blst Lifecycle to State ([#516](#516)) ([0a9c179](0a9c179)) * **bindings:** use zapi js.io() instead of local io module ([#469](#469)) ([2b34cc0](2b34cc0)) * **bindings:** wake only required number of workers ([#383](#383)) ([1db57f1](1db57f1)) * **bls:** allocations around VMAS ([#395](#395)) ([dfda58c](dfda58c)) * **bls:** clean up bls ([#398](#398)) ([e0f3b9b](e0f3b9b)) * **bls:** remove need for tracking results for verifyMultipleAggregateSignatures ([#389](#389)) ([6fe5c3f](6fe5c3f)) * **bls:** remove single-threaded fallback ([#390](#390)) ([e057713](e057713)) * **clock:** single public Clock; internalize SlotClock ([#463](#463)) ([fbab1fa](fbab1fa)) * make XXXDecisionRoot fns return `js.String` ([#342](#342)) ([aef4420](aef4420)) * move shuffle into swap_or_not_shuffle module ([#558](#558)) ([e56efb2](e56efb2)) * **pubkeys:** centralize the process-wide cache ([#522](#522)) ([dc9669d](dc9669d)) ### Miscellaneous Chores * avoid slow tests in AGENTS.md ([#546](#546)) ([c60f2a9](c60f2a9)) * bump zapi to include musl build ([#485](#485)) ([0b488cc](0b488cc)) * **ci:** pin github actions with sha hashes ([#507](#507)) ([167b8f5](167b8f5)) * deprecate unused blst APIs ([#575](#575)) ([7b547fa](7b547fa)) * **deps:** bump zapi v2.1.0 -> v2.2.0 ([#376](#376)) ([0c240d8](0c240d8)) * **deps:** bump zbuild ([#403](#403)) ([e2545de](e2545de)) * **deps:** compile blst with ReleaseFast ([#391](#391)) ([753a896](753a896)) * **deps:** update zapi to 3.1.0 ([#483](#483)) ([f3e5827](f3e5827)) * **deps:** use zapi v2.1.0 ([#372](#372)) ([88f403a](88f403a)) * disable gemini auto code review ([#382](#382)) ([63e42a4](63e42a4)), closes [#380](#380) * **docs:** add comments section in AGENTS.md ([#566](#566)) ([0c09750](0c09750)) * move state clones out of benchmark run functions ([#324](#324)) ([e4035de](e4035de)) * prepare 1.0.0 release ([#576](#576)) ([20b657b](20b657b)) * release v0.1.2-rc.3 ([#370](#370)) ([e4fc551](e4fc551)) * **release:** 0.1.2-rc.2 ([#365](#365)) ([7046128](7046128)) * **release:** v0.1.2-rc.10 ([#477](#477)) ([9a4fad5](9a4fad5)) * **release:** v0.1.2-rc.4 ([#373](#373)) ([09468f1](09468f1)) * **release:** v0.1.2-rc.5 ([#374](#374)) ([f344efa](f344efa)) * **release:** v0.1.2-rc.6 ([#375](#375)) ([bdf5b67](bdf5b67)) * **release:** v0.1.2-rc.8 ([#401](#401)) ([06f91c2](06f91c2)) * **release:** v0.1.2-rc.9 ([#404](#404)) ([6024800](6024800)) * remove merge transition code ([#359](#359)) ([09b175d](09b175d)) * remove stale epoch cache TODOs ([#534](#534)) ([27a547a](27a547a)) * rename era shortHistoricalRoot to shortEraRoot ([#473](#473)) ([c75a4d3](c75a4d3)) * **scripts:** build bindings with preset ([#434](#434)) ([a1b5ef7](a1b5ef7)) * silence debug log when used in release builds ([#486](#486)) ([c5377d7](c5377d7)) * support dev workflow ([#364](#364)) ([fcb9a78](fcb9a78)) * update gloas types to align with the latest specs ([#431](#431)) ([1f065b5](1f065b5)) * update spec test version to v1.7.0-alpha.11 ([#451](#451)) ([5875660](5875660)) * update spec-test-version: v1.6.0-beta.2 -> v1.7.0-alpha.10 ([#441](#441)) ([f932b1c](f932b1c)) * update zapi to 4.0.0 ([#571](#571)) ([de8e3fd](de8e3fd)) ### Documentation * document security threat model ([#557](#557)) ([e678b87](e678b87)) * more comprehensive AGENTS.md ([#520](#520)) ([c74b386](c74b386)) * **pkix:** document load provenance requirement ([#556](#556)) ([37e0aa2](37e0aa2)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
asyncAggregateWithRandomnesswas declared as apub fninbindings/napi/blst.zigand re-exported frombindings/src/blst.js, but at runtime the JS-side binding wasundefined. This PR rewrites the function to use the zapi DSL so it is actually registered as a JS export.Root cause
zapi's
exportModuleauto-registration (export_module.zig,wrap_function.zig:isDslOrOptionalDsl) only picks up apub fnwhen all of its parameters are DSL types (napi.Value, DSL wrappers likejs.Array, or class types). The previous signatureuses raw NAPI types, so the function was silently skipped at registration. Because Zig is lazy about analyzing unreferenced functions, this also hid several internal bugs in the body that would otherwise have failed to compile.
Test plan
pnpm testzig build build-lib:bindings -Doptimize=ReleaseSafebuilds cleanly.