Native bandersnatch bindings #30
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe PR restructures the Bandersnatch VRF project from a single crate into a three-member Rust workspace (core, wasm-binding, native-binding). It introduces multi-platform native builds via updated CI workflows, implements a new publishing pipeline for platform-specific packages, and adds TypeScript coordination layer that selects native or WASM bindings at runtime based on environment. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
npm workspaces don't respect os/cpu restrictions, causing npm ci to fail on Linux when trying to install darwin-arm64 packages. The platform packages are build artifacts, not development dependencies.
There was a problem hiding this comment.
Pull request overview
This pull request introduces native Node.js bindings for the Bandersnatch VRF library with automatic platform detection and WASM fallback. The implementation demonstrates a ~2x performance improvement over WASM (as shown in the benchmark data), while maintaining backward compatibility through optional dependencies.
Changes:
- Refactored Bandersnatch Rust code into a core library shared by both WASM and native bindings
- Added N-API native bindings for Node.js (darwin-arm64 and linux-x64-gnu)
- Implemented TypeScript wrapper with automatic native/WASM binding selection
- Updated CI/CD workflows to build and publish both native and WASM artifacts
- Version bump from 0.0.4 to 0.2.0 across all packages
Reviewed changes
Copilot reviewed 27 out of 38 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump to 0.2.0, added @napi-rs/cli dependency and new workspace for native bindings |
| package-lock.json | Updated lockfile with new dependencies and workspace entries for native packages |
| bandersnatch/core/ | New shared Rust core library with FFI-ready functions |
| bandersnatch/native-binding/ | New N-API native bindings using napi-rs |
| bandersnatch/wasm-binding/ | Extracted WASM-specific bindings from main package |
| bandersnatch/src/index.ts | New TypeScript wrapper with automatic native/WASM selection |
| bandersnatch/npm/ | Platform-specific native packages (darwin-arm64, linux-x64-gnu, native wrapper) |
| native/index.ts | Updated WASM import path to point to new wasm-binding location |
| .github/workflows/ | Updated CI/CD to build native bindings on multiple platforms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/publish.yml:
- Around line 36-66: The workflow references matrix.settings.package in the
Upload artifact step but the matrix only defines host and target; update the
build-native matrix entries to include a package key for each settings entry
(e.g., add package: <npm-package-dir-name> alongside host and target) so ${{
matrix.settings.package }} resolves correctly in the Upload artifact path, or if
you prefer not to extend the matrix, change the Upload artifact path to a
concrete path that does not rely on matrix.settings.package; edit the matrix
definition near build-native (matrix.settings) and the Upload artifact step
(uses: actions/upload-artifact@v4) accordingly.
In `@bandersnatch/npm/native/package.json`:
- Around line 5-18: The package.json declares "type": "module" but the CJS entry
points to index.js which is parsed as ESM; rename the CommonJS implementation
file index.js to index.cjs and update package.json: change "main" to
"./index.cjs" and update the exports map so the "require".default points to
"./index.cjs" (leave "module" -> "./index.mjs" and "types" unchanged) so ESM and
CJS consumers load the correct files.
In `@bandersnatch/src/index.ts`:
- Around line 31-37: The wasm init call in loadWasmBinding is incorrectly
passing an object wrapper; update the call to pass the WasmBinding.InitInput
directly to the generated init function. Locate loadWasmBinding and replace the
call to wasmBindingModule.default that currently wraps wasmModule in an object
(referenced as wasmBindingModule.default) so it instead calls
wasmBindingModule.default with wasmModule as the sole argument.
🧹 Nitpick comments (12)
native/tsconfig.json (1)
1-14: Consider a broader include glob to catch nested TS files.Using
"include": ["*.ts"]won’t typecheck files in subdirectories if the native package grows. A broader glob reduces maintenance overhead.♻️ Suggested tweak
- "include": ["*.ts"], + "include": ["**/*.ts"], "files": ["env.d.ts"]bandersnatch/core/Cargo.toml (1)
14-15: Redundant dev-dependency.
hexis already declared as a regular dependency on line 11 with the same version. The dev-dependency entry is unnecessary since regular dependencies are also available in tests.🔧 Suggested fix
-[dev-dependencies] -hex = "0.4.3" - [features]native/index.ts (1)
29-33: Consider simplifying async wrappers.The double
async/awaitis slightly redundant since the inner functions already return promises. This is a minor style preference and works correctly as-is.🔧 Optional simplification
export const init = { - bandersnatch: initOnce(async () => await bandersnatchInit({ module_or_path: await bandersnatchWasm() })), - ed25519: initOnce(async () => await ed25519Init({ module_or_path: await ed25519Wasm() })), - reedSolomon: initOnce(async () => await reedSolomonInit({ module_or_path: await reedSolomonWasm() })), + bandersnatch: initOnce(async () => bandersnatchInit({ module_or_path: await bandersnatchWasm() })), + ed25519: initOnce(async () => ed25519Init({ module_or_path: await ed25519Wasm() })), + reedSolomon: initOnce(async () => reedSolomonInit({ module_or_path: await reedSolomonWasm() })), };bandersnatch/build-native.sh (2)
35-39: Consider adding artifact existence check before copy.If the cargo build succeeds but produces an unexpected output path or filename, the
cpwill fail with a less informative error. Adding a check improves debuggability.🔧 Suggested improvement
artifact="$target_dir/release/libbandersnatch_native.$lib_ext" dest_dir="$ROOT/npm/$package" +if [ ! -f "$artifact" ]; then + echo "Error: Expected artifact not found at $artifact" + exit 1 +fi + mkdir -p "$dest_dir" cp "$artifact" "$dest_dir/bandersnatch.$package.node"
20-33: Limited platform support.The script only supports
aarch64-apple-darwinandx86_64-unknown-linux-gnu. Consider documenting this limitation or adding support for additional common targets (e.g.,x86_64-apple-darwinfor Intel Macs,aarch64-unknown-linux-gnufor ARM Linux) as the project grows.bandersnatch/npm/native/index.js (1)
1-1: Remove unused import.
createRequireis imported from themodulepackage but never used in this file. The ESM version (index.mjs) does use it, but this CommonJS version usesrequiredirectly.🔧 Suggested fix
-const { createRequire } = require('module'); - function loadNativeBinding() {.github/workflows/publish.yml (1)
104-106: Minor: Consider quoting shell variable.Static analysis flagged SC2086 on
git rev-parse --short HEAD. While git short SHAs are alphanumeric and safe, quoting is a good defensive practice.🔧 Suggested fix
- id: vars - run: echo "short_sha=$(git rev-parse --short HEAD)" >> $GITHUB_OUTPUT + run: echo "short_sha=$(git rev-parse --short HEAD)" >> "$GITHUB_OUTPUT"bandersnatch/npm/native/index.mjs (1)
12-31: Confirm libc/OS coverage for Linux targets.On Alpine/musl,
linux+x64will still try the glibc binary, which typically fails at load time. If musl is unsupported, detect it and throw an explicit “unsupported platform” error; if it is supported, add a musl mapping to avoid confusing loader errors.bandersnatch/package.json (1)
8-27: Verify ESM-only export surface is intentional.The
exportsmap only definesimport, and with"type": "module"anyrequire('@typeberry/bandersnatch')will fail. If CJS consumers are still supported, add arequirecondition and a CJS build/wrapper; otherwise document the breaking change.bandersnatch/src/index.ts (1)
84-97: Initialization is idempotent butInitOptionsignored on re-init.When already initialized, the function returns early without applying
options. This is correct behavior but could be documented for clarity. Consider adding a JSDoc note that options are only used on first init.📝 Optional: Add JSDoc clarification
+/** + * Initialize the Bandersnatch binding. + * Options are only applied on the first call; subsequent calls return the existing API. + */ export default async function init(options?: InitOptions): Promise<BandersnatchApi> {bandersnatch/core/src/lib.rs (2)
34-42:RingSize::from_sizesilently defaults toTinyfor unrecognized sizes.Any size that doesn't match
Full.size()(1023) will default toTiny. This could mask configuration errors. Consider returning aResultorOption, or at minimum document this behavior.📝 Consider explicit error handling or documentation
/// Create a RingSize from a numeric size. + /// Returns `Tiny` if size doesn't match a known ring size. pub fn from_size(size: usize) -> Self { if size == Self::Full.size() { RingSize::Full + } else if size == Self::Tiny.size() { + RingSize::Tiny } else { - RingSize::Tiny + // Log warning or consider returning Option<RingSize> + RingSize::Tiny // Default fallback } }
107-111:deserialize_public_keysilently returns padding point on failure.This silent fallback to a padding point could mask deserialization errors. While this may be intentional for ring operations, consider documenting this behavior explicitly.
📝 Add documentation for fallback behavior
-/// Deserialize a public key from bytes, returning the padding point on failure. +/// Deserialize a public key from bytes. +/// +/// Returns the ring padding point on deserialization failure, which allows +/// invalid keys to be safely included in ring operations without causing panics. pub fn deserialize_public_key(chunk: &[u8]) -> Public {
Uh oh!
There was an error while loading. Please reload this page.