feat: support bitArrays in to_napi_value#368
Conversation
`toValue()` requires support for converting bit arrays into napi values
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 extends the N-API conversion logic to include support for bit arrays. By implementing a dedicated conversion path for bit vectors and bit lists, the changes ensure that these data structures are correctly represented when passed across the FFI boundary, maintaining interoperability with existing TypeScript implementations. 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
|
![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 adds support for bit vectors and bit lists in the N-API bindings by introducing the bitArrayToNapiValue helper function and updating the sszValueToNapiValue logic. Feedback was provided to improve the robustness of the new function by adding assertions for argument validation, specifically to ensure that bit_len is within the range of an i64 and that the data slice length is consistent with the bit length, as required by the repository's style guide.
| fn bitArrayToNapiValue(env: napi.Env, data: []const u8, bit_len: usize) !napi.Value { | ||
| var bytes: [*]u8 = undefined; | ||
| const buf = try env.createArrayBuffer(data.len, &bytes); | ||
| @memcpy(bytes[0..data.len], data); | ||
| const uint8_array = try env.createTypedarray(.uint8, data.len, buf, 0); | ||
|
|
||
| const obj = try env.createObject(); | ||
| try obj.setNamedProperty("uint8Array", uint8_array); | ||
| try obj.setNamedProperty("bitLen", try env.createInt64(@intCast(bit_len))); | ||
| return obj; | ||
| } |
There was a problem hiding this comment.
The bitArrayToNapiValue function is missing assertions for its arguments, which is a requirement of the repository's style guide (based on TigerStyle). Specifically, it should assert that bit_len is within the range of i64 (since it is cast to it for the N-API call) and that the data slice length is consistent with the provided bit_len.
According to the style guide:
- "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."
- "Split compound assertions: prefer
assert(a); assert(b);overassert(a and b);."
fn bitArrayToNapiValue(env: napi.Env, data: []const u8, bit_len: usize) !napi.Value {
std.debug.assert(bit_len <= std.math.maxInt(i64));
std.debug.assert(data.len == (bit_len + 7) / 8);
var bytes: [*]u8 = undefined;
const buf = try env.createArrayBuffer(data.len, &bytes);
@memcpy(bytes[0..data.len], data);
const uint8_array = try env.createTypedarray(.uint8, data.len, buf, 0);
const obj = try env.createObject();
try obj.setNamedProperty("uint8Array", uint8_array);
try obj.setNamedProperty("bitLen", try env.createInt64(@intCast(bit_len)));
return obj;
}
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. Split compound assertions: prefer assert(a); assert(b); over assert(a and b);. (link)
There was a problem hiding this comment.
accepted the assertion for data.len
|
I think we could do with unit tests for this fn at some point in a future PR |
|
One thing I'm not sure how to handle here. |
there's a bunch of mismatch between whether we pass raw bytes or pass objects in some APIs in our bindings, including (but not limited to) calls related to voluntary exits (see #378) and blocks (see #379) I thought about it a while and chose to prioritize lodestar and assume we're handling objects on the zig side, at least for the initial integration to reduce diff for better reviewability. That's why in some of the PRs i broke off from #347 i'm manually walking the napi object instead of relying on the serializer/deserializer APIs This isn't exactly the same issue but in a similar vein if we deal with raw bytes we can leave the burden up to the consumer ( |
**Motivation** - it's tricky to use BitArray, which is defined in ssz for binding, see this [concern](ChainSafe/lodestar-z#368 (comment)) **Description** - the native binding does not have to do anything with `BitArray`, use `{uint8Array: Uint8Array; bitLen: number}` instead, the binding needs to conform to`IBeaconStateViewNative` overall - implement `NativeBeaconStateView` wrapper that conform to the public api of `IBeaconStateViewLatestFork` so `beacon-node` does not need to change. It also contains a cache layer so that it does not need to fetch native multiple times for the same data --------- Co-authored-by: twoeths <twoeths@users.noreply.github.com>
2 participants
extracted from #347
toValue()requires support for converting bit arrays into napi values