Skip to content

refactor: make XXXDecisionRoot fns return js.String #342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
spiral-ladder merged 2 commits into from
May 5, 2026
Merged

refactor: make XXXDecisionRoot fns return js.String #342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9038c98
refactor: make XXXDecisionRoot fns return `js.String`
spiral-ladder May 1, 2026
a7c8844
update root related tests
spiral-ladder May 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 16 additions & 14 deletions bindings/napi/BeaconStateView.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,37 +371,39 @@ pub fn proposerLookahead(self: *const BeaconStateView) !js.Uint32Array {

// pub fn BeaconStateView_getShufflingAtEpoch

pub fn previousDecisionRoot(self: *const BeaconStateView) !js.Uint8Array {
fn rootToHexString(root: *const [32]u8) !js.String {
const env = js.env();
var hex_buf: [66]u8 = undefined;
try @import("hex").rootIntoHex(&hex_buf, root);
return js_types.wrap(js.String, try env.createStringUtf8(&hex_buf));
}

pub fn previousDecisionRoot(self: *const BeaconStateView) !js.String {
const cached_state = try self.requireState();
const root = cached_state.previousDecisionRoot();
return js_types.wrap(js.Uint8Array, try sszValueToNapiValue(env, ct.primitive.Root, &root));
return rootToHexString(&root);
}

Comment on lines +374 to 386

@gemini-code-assist gemini-code-assist Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The original function signature at line 374 was not removed, resulting in a dangling signature followed by a new function definition. This will cause a compilation error. Additionally, the repository style guide (lines 51-55) requires a minimum of two assertions per function to enforce invariants and detect programmer errors. I have included suggested assertions for the helper and the function body below.

fn rootToHexString(root: *const [32]u8) !js.String {
    const env = js.env();
    var hex_buf: [66]u8 = undefined;
    try @import("hex").rootIntoHex(&hex_buf, root);
    const result = try env.createStringUtf8(&hex_buf);
    std.debug.assert(hex_buf.len == 66);
    std.debug.assert(root.len == 32);
    return js_types.wrap(js.String, result);
}

pub fn previousDecisionRoot(self: *const BeaconStateView) !js.String {
    const cached_state = try self.requireState();
    std.debug.assert(self.cached_state != null);
    const root = cached_state.previousDecisionRoot();
    std.debug.assert(root.len == 32);
    return rootToHexString(&root);
}
References
  1. 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)

@spiral-ladder spiral-ladder May 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outdated; fixed in new push

pub fn currentDecisionRoot(self: *const BeaconStateView) !js.Uint8Array {
const env = js.env();
pub fn currentDecisionRoot(self: *const BeaconStateView) !js.String {
const cached_state = try self.requireState();
const root = cached_state.currentDecisionRoot();
return js_types.wrap(js.Uint8Array, try sszValueToNapiValue(env, ct.primitive.Root, &root));
return rootToHexString(&root);
}

Comment on lines 388 to 392

@gemini-code-assist gemini-code-assist Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function is missing the required assertions per the repository style guide (lines 51-55). Each function should have at least two assertions to verify invariants and state.

pub fn currentDecisionRoot(self: *const BeaconStateView) !js.String {
    const cached_state = try self.requireState();
    std.debug.assert(self.cached_state != null);
    const root = cached_state.currentDecisionRoot();
    std.debug.assert(root.len == 32);
    return rootToHexString(&root);
}
References
  1. The assertion density of the code must average a minimum of two assertions per function. (link)

/// Get the next decision root for the state.
pub fn nextDecisionRoot(self: *const BeaconStateView) !js.Uint8Array {
const env = js.env();
pub fn nextDecisionRoot(self: *const BeaconStateView) !js.String {
const cached_state = try self.requireState();
const root = cached_state.nextDecisionRoot();
return js_types.wrap(js.Uint8Array, try sszValueToNapiValue(env, ct.primitive.Root, &root));
return rootToHexString(&root);
}

Comment on lines 395 to 399

@gemini-code-assist gemini-code-assist Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This function is missing the required assertions per the repository style guide (lines 51-55).

pub fn nextDecisionRoot(self: *const BeaconStateView) !js.String {
    const cached_state = try self.requireState();
    std.debug.assert(self.cached_state != null);
    const root = cached_state.nextDecisionRoot();
    std.debug.assert(root.len == 32);
    return rootToHexString(&root);
}
References
  1. The assertion density of the code must average a minimum of two assertions per function. (link)

/// Get the shuffling decision root for a given epoch.
pub fn getShufflingDecisionRoot(self: *const BeaconStateView, epoch_arg: js.Number) !js.Uint8Array {
const env = js.env();
pub fn getShufflingDecisionRoot(self: *const BeaconStateView, epoch_arg: js.Number) !js.String {
const cached_state = try self.requireState();
const epoch_value: u64 = @intCast(try epoch_arg.toI64());

@spiral-ladder spiral-ladder May 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

u32 autowidens to u64 when used as an arg into calculateShufflingDecisionRoot, so changed it to inline

this is somewhat of a unrelated change to this PR but also a small change so i did a driveby

const root = st.calculateShufflingDecisionRoot(cached_state.state, epoch_value) catch {
return throwNullAs(js.Uint8Array, "STATE_ERROR", "Failed to calculate shuffling decision root");
const root = st.calculateShufflingDecisionRoot(cached_state.state, try epoch_arg.toU32()) catch {
return throwNullAs(js.String, "STATE_ERROR", "Failed to calculate shuffling decision root");
};
return js_types.wrap(js.Uint8Array, try sszValueToNapiValue(env, ct.primitive.Root, &root));
return rootToHexString(&root);
}

pub fn previousProposers(self: *const BeaconStateView) !?js.Array {
Comment on lines 402 to 409

@gemini-code-assist gemini-code-assist Bot May 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of toU32() for the epoch value is inconsistent with the rest of the file, which uses toI64() and @intCast to u64. Since Ethereum epochs are 64-bit values, using toU32() introduces an unnecessary limitation and breaks consistency with other getters in this class. Additionally, the function lacks the required assertions per the repository style guide.

pub fn getShufflingDecisionRoot(self: *const BeaconStateView, epoch_arg: js.Number) !js.String {
    const cached_state = try self.requireState();
    std.debug.assert(self.cached_state != null);
    const epoch_value: u64 = @intCast(try epoch_arg.toI64());
    const root = st.calculateShufflingDecisionRoot(cached_state.state, epoch_value) catch {
        return throwNullAs(js.String, "STATE_ERROR", "Failed to calculate shuffling decision root");
    };
    std.debug.assert(root.len == 32);
    return rootToHexString(&root);
}
References
  1. The assertion density of the code must average a minimum of two assertions per function. (link)

Expand Down
12 changes: 6 additions & 6 deletions bindings/test/beaconStateView.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -397,15 +397,15 @@ describe("BeaconStateView", () => {
expect(proposer).toBeLessThan(state.validatorCount);
});

it("decision roots should be 32 bytes each", () => {
expect(state.previousDecisionRoot.length).toBe(32);
expect(state.currentDecisionRoot.length).toBe(32);
expect(state.nextDecisionRoot.length).toBe(32);
it("decision roots should be 66 bytes each", () => {
expect(state.previousDecisionRoot.length).toBe(66);
expect(state.currentDecisionRoot.length).toBe(66);
expect(state.nextDecisionRoot.length).toBe(66);
});

it("getShufflingDecisionRoot should return 32 bytes", () => {
it("getShufflingDecisionRoot should return 66 bytes", () => {
const decisionRoot = state.getShufflingDecisionRoot(state.epoch);
expect(decisionRoot.length).toBe(32);
expect(decisionRoot.length).toBe(66);
});
});

Expand Down
1 change: 1 addition & 0 deletions build.zig.zon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@
.imports = .{
.bls,
.bls_options,
.hex,
.persistent_merkle_tree,
.ssz,
.consensus_types,
Expand Down
Loading