feat: use high level zapi dsl #302
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 migrates the existing N-API bindings to the new high-level ZAPI DSL. This change aims to reduce boilerplate code, enhance type safety, and ensure more consistent development across the bindings. The update includes necessary adjustments to build configurations, module registration, and test suites to accommodate the new DSL structure. 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 refactors the N-API bindings to use a new DSL and the zapi module structure, moving away from manual registration in favor of js.exportModule. Key changes include updating import paths, refactoring functions to use zapi.js types (such as js.Value, js.Number, and js.Uint8Array), and adjusting the pubkeys cache size from a property to a function. Feedback focuses on adhering to the repository's strict style guide, specifically regarding assertion density (minimum two per function), function length limits (70 lines), avoiding variable abbreviations, and removing redundant type casts.
|
|
||
| pub fn Config_set(env: napi.Env, cb: napi.CallbackInfo(2)) !napi.Value { | ||
| /// JS: config.set(chainConfigObj, genesisValidatorsRoot) | ||
| pub fn set(obj: js.Value, genesis_root: js.Uint8Array) !void { |
There was a problem hiding this comment.
This function lacks assertions. The repository style guide requires that all function arguments, return values, pre/postconditions, and invariants be asserted. Furthermore, the assertion density must average a minimum of two assertions per function.
References
- Assert all function arguments and return values, pre/postconditions and invariants. The assertion density of the code must average a minimum of two assertions per function. (link)
There was a problem hiding this comment.
Arguments are asserted inside the functions.
| } | ||
|
|
||
| pub fn chainConfigFromObject(env: napi.Env, obj: napi.Value) !ChainConfig { | ||
| fn chainConfigFromObject(env: napi.Env, obj: napi.Value) !ChainConfig { |
There was a problem hiding this comment.
The function chainConfigFromObject exceeds the hard limit of 70 lines (it is currently 80 lines long). Consider refactoring this function by splitting the complex object parsing into smaller, specialized helper functions.
References
- Restrict the length of function bodies to reduce the probability of poorly structured code. We enforce a hard limit of 70 lines per function. (link)
There was a problem hiding this comment.
Out of scope from this PR.
| } | ||
|
|
||
| return try env.createUint32(@intCast(state.index2pubkey.items.len)); | ||
| return js.Number.from(@as(u32, @intCast(state.index2pubkey.items.len))); |
There was a problem hiding this comment.
The @as(u32, ...) cast is redundant here. Since @intCast is used within the context of a function call where the expected type can be inferred, the explicit type attribution is unnecessary.
return js.Number.from(@intCast(state.index2pubkey.items.len));
There was a problem hiding this comment.
js.Number.from accepts anytype so we have to use @as for proper typecast.
| return env.getUndefined(); | ||
| } | ||
|
|
||
| pub fn chainConfigFromObject(env: napi.Env, obj: napi.Value) !ChainConfig { |
There was a problem hiding this comment.
As the pub is removed, this function will not be exposed to binding.
| return error.PoolNotInitialized; | ||
| } | ||
|
|
||
| const requested = new_size.assertU32(); |
There was a problem hiding this comment.
I don't see that the DSL contract enforces that new_size is a number? so using assertXX here is not ideal and can cause a crash (when it should just throw an error).
There was a problem hiding this comment.
The type new_size: js.Number enforces it's a number, but it can be a uint or int, as JS number support both. So we need to assert here.
This is same as std.debug.assert for function arguments.
Resolve conflicts with 5c50ab4 (chore: fix zapi usage #307). Main's changes (import rename to zapi:zapi, env.wrap extra param, error message update) are all superseded by the DSL rewrite — kept our version for all 15 conflicted files. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
Closing in favor of #320 |
2 participants
To make the development of the bindings easy and consistent use the new DSL introduced in ChainSafe/zapi#11