Bug/41 improve package structure#45
Conversation
Bug/36 dont use buffer
There was a problem hiding this comment.
Pull request overview
This PR aims to address #41 by reducing Node-only Buffer usage in core encryption code, exposing byte utilities, and adding cross-environment smoke tests (Node/Web/Workers/Bun/Deno) that are run in CI.
Changes:
- Add
src/utils/bytes.tsand exporthexToBytes/bytesToHex/concatBytesfrom the package entrypoint. - Replace
Buffer-based RLP/hex conversions in encryption paths (and one test) withUint8Array-based helpers. - Add environment smoke test harnesses (web + Cloudflare Worker + Node/Bun/Deno scripts) and wire them into
.github/workflows/test.yml.
Reviewed changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_epoch_validation.js | Switches from Buffer to exported byte helpers for RLP/hex handling in the test. |
| src/utils/bytes.ts | Introduces hex/byte conversion + concatenation helpers with hex validation. |
| src/index.ts | Exports the new byte helpers as part of the public API. |
| src/core/encrypt.ts | Replaces Buffer.from(..., 'hex')/Buffer.toString('hex') with hexToBytes/bytesToHex for RLP encoding. |
| scripts/env-smoke/worker/wrangler.toml | Adds a Wrangler config for the Worker smoke test. |
| scripts/env-smoke/worker/src/index.js | Adds a Worker endpoint that runs encryption and validates the RLP payload shape. |
| scripts/env-smoke/worker/runtime-check.mjs | Adds a Node runner that starts wrangler dev and probes for a PASS response. |
| scripts/env-smoke/worker/package.json | Adds a standalone npm project for the Worker smoke test. |
| scripts/env-smoke/worker/package-lock.json | Locks dependencies for the Worker smoke test project. |
| scripts/env-smoke/web/vite.config.js | Adds Vite config for the web smoke test. |
| scripts/env-smoke/web/src/main.js | Adds browser smoke test logic that runs encryption and validates the RLP payload. |
| scripts/env-smoke/web/runtime-check.mjs | Adds a Playwright-based runtime check for the web smoke test. |
| scripts/env-smoke/web/package.json | Adds a standalone npm project for the web smoke test (Vite + Playwright). |
| scripts/env-smoke/web/package-lock.json | Locks dependencies for the web smoke test project. |
| scripts/env-smoke/web/index.html | Adds minimal page for displaying PASS/FAIL status. |
| scripts/env-smoke/shared-fixture.mjs | Adds shared fixture values and helper functions for smoke tests. |
| scripts/env-smoke/node-smoke.mjs | Adds Node ESM smoke test script validating encryption output. |
| scripts/env-smoke/node-smoke.cjs | Adds Node CJS smoke test script validating encryption output. |
| scripts/env-smoke/deno-smoke.ts | Adds Deno smoke test script validating encryption output. |
| scripts/env-smoke/bun-smoke.mjs | Adds Bun smoke test script validating encryption output. |
| scripts/env-smoke/README.md | Documents how to run each environment smoke test locally. |
| scripts/endpoint_by_container/data_dir/config.json | Adds patch timestamps used by containerized test/config flows. |
| package.json | Bumps package version and adds npm scripts to run environment smoke tests. |
| examples/confidential-token-demo/src/encryption.js | Updates demo code to use byte helpers instead of Node Buffer APIs. |
| .github/workflows/test.yml | Adds Bun/Deno setup and runs the new environment smoke tests in CI; bumps container tags. |
Files not reviewed (2)
- scripts/env-smoke/web/package-lock.json: Language not supported
- scripts/env-smoke/worker/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function hexToBytes(hex) { | ||
| const clean = hex.startsWith('0x') ? hex.slice(2) : hex; | ||
| const bytes = new Uint8Array(clean.length / 2); | ||
|
|
||
| for (let i = 0; i < clean.length; i += 2) { |
There was a problem hiding this comment.
hexToBytes / bytesToHex are duplicated here and in the worker smoke test. Since a shared helper module exists (scripts/env-smoke/shared-fixture.mjs) and the library now exports byte utilities, consider reusing one of those to avoid divergence between environments.
| { | ||
| "name": "@skalenetwork/bite", | ||
| "version": "0.8.1", | ||
| "version": "0.8.2", | ||
| "description": "TS Library to interact with BITE protocol", | ||
| "homepage": "https://github.com/skalenetwork/bite.ts", | ||
| "license": "LGPL-3.0-or-later", |
There was a problem hiding this comment.
PR description says it “updates t-encrypt package” / fixes #41, but this repo still depends on @skalenetwork/t-encrypt at 0.8.0-develop.0 (see package.json dependencies). Since npm versions are immutable, the Workers/WASM fix likely requires bumping the dependency to the new published version (or updating the PR description if the t-encrypt change is out-of-scope for this PR).
|
|
||
| const COMMITTEE_INFO = [ | ||
| { | ||
| commonBLSPublicKey: '2d3846dc859e04cf130cd58a6b73f6f94def67adbec30df6dcfe0d6ab7d8a280173fff75e7afa1eaaf98b18282ae1a832475602f27480120adca88f6c3febc5b2b3a808057137751edbd32f3b6ec003fe120c53b133b0c25f2d050f123fb36ef2f2e6876d111b220ef84a86deb891c45571907e19366d405692f59e46f443d02', | ||
| epochId: 777 | ||
| } | ||
| ]; | ||
|
|
||
| const SAMPLE_TX = { | ||
| to: '0x1234567890123456789012345678901234567890', | ||
| data: '0x1234abcd' | ||
| }; | ||
|
|
There was a problem hiding this comment.
COMMITTEE_INFO (and the related SAMPLE_TX below) are duplicated across the env smoke targets (web/worker/node). Since scripts/env-smoke/shared-fixture.mjs already exists in this PR, consider importing the fixture from there to keep the test vectors consistent and reduce maintenance overhead.
| const COMMITTEE_INFO = [ | |
| { | |
| commonBLSPublicKey: '2d3846dc859e04cf130cd58a6b73f6f94def67adbec30df6dcfe0d6ab7d8a280173fff75e7afa1eaaf98b18282ae1a832475602f27480120adca88f6c3febc5b2b3a808057137751edbd32f3b6ec003fe120c53b133b0c25f2d050f123fb36ef2f2e6876d111b220ef84a86deb891c45571907e19366d405692f59e46f443d02', | |
| epochId: 777 | |
| } | |
| ]; | |
| const SAMPLE_TX = { | |
| to: '0x1234567890123456789012345678901234567890', | |
| data: '0x1234abcd' | |
| }; | |
| import { COMMITTEE_INFO, SAMPLE_TX } from '../../shared-fixture.mjs'; |
| "dependencies": { | ||
| "@skalenetwork/bite": "file:../../.." | ||
| }, | ||
| "devDependencies": { | ||
| "playwright": "^1.53.0", | ||
| "vite": "^6.3.1" | ||
| } |
There was a problem hiding this comment.
@ethereumjs/rlp is imported directly by src/main.js, but it isn’t listed in this package’s dependencies/devDependencies. Relying on @skalenetwork/bite’s transitive dependency can break installs (and bundlers) if the dependency tree changes; please add @ethereumjs/rlp explicitly.
| "dependencies": { | ||
| "@skalenetwork/bite": "file:../../.." | ||
| }, | ||
| "devDependencies": { | ||
| "wrangler": "^3.99.0" | ||
| } |
There was a problem hiding this comment.
@ethereumjs/rlp is imported directly by the worker entry (src/index.js), but it isn’t listed in this package’s dependencies/devDependencies. Please add it explicitly to avoid relying on transitive deps (which can break wrangler/esbuild resolution if the tree changes).
| const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm'; | ||
| const worker = spawn(npmCmd, ['run', 'dev'], { | ||
| cwd: new URL('.', import.meta.url), | ||
| stdio: 'pipe' | ||
| }); |
There was a problem hiding this comment.
spawn(..., { cwd: new URL('.', import.meta.url) }) passes a URL object as cwd. In Node 18 (used in CI), child_process expects a filesystem path string here, so this can throw at runtime. Convert to a path with fileURLToPath(new URL('.', import.meta.url)) (or similar) before passing to spawn.
| const npmCmd = process.platform === 'win32' ? 'npm.cmd' : 'npm'; | ||
| const cwd = new URL('.', import.meta.url); | ||
|
|
||
| await runCommandAndWait(npmCmd, ['run', 'build'], { cwd }); | ||
|
|
||
| const server = runCommand( | ||
| npmCmd, | ||
| ['run', 'preview', '--', '--host', HOST, '--port', String(PORT), '--strictPort'], | ||
| { cwd } | ||
| ); |
There was a problem hiding this comment.
cwd is set to a URL (new URL('.', import.meta.url)) and then passed to spawn. On Node 18 this is not consistently supported (it expects a path string), which can break the smoke test in CI. Please convert the file URL to a filesystem path (e.g. via fileURLToPath) before using it as cwd.
| function hexToBytes(hex) { | ||
| const clean = hex.startsWith('0x') ? hex.slice(2) : hex; | ||
| const bytes = new Uint8Array(clean.length / 2); | ||
|
|
||
| for (let i = 0; i < clean.length; i += 2) { |
There was a problem hiding this comment.
Helper utilities (hexToBytes / bytesToHex / assert) are redefined here even though similar helpers exist in scripts/env-smoke/shared-fixture.mjs (and @skalenetwork/bite now exports byte helpers). Reusing the shared helper reduces duplication and helps keep behavior identical between smoke targets.
|
|
||
| const COMMITTEE_INFO = [ | ||
| { | ||
| commonBLSPublicKey: '2d3846dc859e04cf130cd58a6b73f6f94def67adbec30df6dcfe0d6ab7d8a280173fff75e7afa1eaaf98b18282ae1a832475602f27480120adca88f6c3febc5b2b3a808057137751edbd32f3b6ec003fe120c53b133b0c25f2d050f123fb36ef2f2e6876d111b220ef84a86deb891c45571907e19366d405692f59e46f443d02', | ||
| epochId: 777 | ||
| } | ||
| ]; | ||
|
|
||
| const SAMPLE_TX = { | ||
| to: '0x1234567890123456789012345678901234567890', | ||
| data: '0x1234abcd' | ||
| }; | ||
|
|
There was a problem hiding this comment.
COMMITTEE_INFO (and the related SAMPLE_TX below) are duplicated across the env smoke targets (web/worker/node). Since scripts/env-smoke/shared-fixture.mjs was added in this PR, consider importing the fixture from there so the test vectors stay in sync.
| const COMMITTEE_INFO = [ | |
| { | |
| commonBLSPublicKey: '2d3846dc859e04cf130cd58a6b73f6f94def67adbec30df6dcfe0d6ab7d8a280173fff75e7afa1eaaf98b18282ae1a832475602f27480120adca88f6c3febc5b2b3a808057137751edbd32f3b6ec003fe120c53b133b0c25f2d050f123fb36ef2f2e6876d111b220ef84a86deb891c45571907e19366d405692f59e46f443d02', | |
| epochId: 777 | |
| } | |
| ]; | |
| const SAMPLE_TX = { | |
| to: '0x1234567890123456789012345678901234567890', | |
| data: '0x1234abcd' | |
| }; | |
| import { COMMITTEE_INFO, SAMPLE_TX } from '../../shared-fixture.mjs'; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 38 changed files in this pull request and generated 1 comment.
Files not reviewed (4)
- scripts/env-smoke/ts-consumer/bundler/package-lock.json: Language not supported
- scripts/env-smoke/ts-consumer/nodenext/package-lock.json: Language not supported
- scripts/env-smoke/web/package-lock.json: Language not supported
- scripts/env-smoke/worker/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| matrix: | ||
| os: [ubuntu-latest, windows-latest, macos-latest] | ||
| node: ['18', '20'] | ||
|
|
There was a problem hiding this comment.
The Node matrix includes windows-latest, but the build runs yarn build, which relies on shelling out to POSIX-style commands (e.g. rm -rf dist via the clean script). This is likely to fail on Windows unless the runner environment happens to provide rm. Consider making the clean step cross-platform (e.g. using a Node-based delete or a dedicated cross-platform tool) or running the build steps under shell: bash on Windows runners.
fixes #41
add tests to run bite-ts in different environments (node, web, etc) and on different platforms (Ubuntu, MacOS, Windows)
update t-encrypt package