Offline networking worker#1026
Conversation
📝 WalkthroughWalkthroughAdds a browser-safe crypto entry point and updates JAM block imports to use it. Introduces the Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bin/lib/README.md`:
- Around line 123-134: Make the README example self-contained by replacing the
two assert.strictEqual calls around importedBlocks and announcedHeader with
explicit browser-safe equality checks, or define an appropriate assertion
dependency within the example. Preserve both validations: each value must equal
the expected block or header.
In `@packages/core/crypto/browser.test.ts`:
- Around line 1-2: Replace the node:assert and node:test imports in
browser.test.ts with the repository’s runtime-neutral test and assertion
utilities. Keep the test compatible with both Node and web runtimes without
renaming it or introducing separate runtime-specific files.
In `@packages/core/crypto/README.md`:
- Around line 6-10: Update the description of the `@typeberry/crypto/browser.js`
entry point to say it is for code that only needs the sizes of encoded
keys/signatures, leaving the surrounding documentation unchanged.
In `@packages/core/listener/index.test.ts`:
- Around line 1-2: Make the test files runtime-neutral by replacing the
Node-only imports in packages/core/listener/index.test.ts (lines 1-2),
packages/jam/networking-offline/index.test.ts (lines 1-2), and
bin/lib/examples/networking-offline.test.ts (lines 1-2) with the repository’s
browser-compatible test and assertion utilities. Keep each file named *.test.ts
so it remains compatible with both Node and web environments; do not rename them
to a Node-only convention.
In `@packages/jam/networking-offline/index.ts`:
- Around line 88-90: Replace assertActive’s exception-based inactive-state
handling with the repository’s Result/tagged-union convention, and update
submitBlock, submitBlocks, and submitTickets to return the tagged lifecycle
failure through their promise results. Adjust the related test to assert the
Result variant rather than a message-specific rejected Error, while preserving
successful active-state behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a5457743-8d8d-4afe-b4e7-71e18b0f8a98
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (28)
bin/lib/README.mdbin/lib/examples/networking-offline.test.tsbin/lib/exports/crypto-browser.tsbin/lib/exports/networking-offline.tsbin/lib/index.tsbin/lib/package.jsonpackage.jsonpackages/core/crypto/README.mdpackages/core/crypto/browser.test.tspackages/core/crypto/browser.tspackages/core/listener/index.test.tspackages/core/listener/index.tspackages/core/listener/package.jsonpackages/core/utils/test.tspackages/jam/block/assurances.tspackages/jam/block/disputes.tspackages/jam/block/guarantees.tspackages/jam/block/header.tspackages/jam/block/tickets.tspackages/jam/networking-offline/README.mdpackages/jam/networking-offline/index.test.tspackages/jam/networking-offline/index.tspackages/jam/networking-offline/package.jsonpackages/workers/api/channel.tspackages/workers/comms-authorship-network/protocol.tspackages/workers/comms-authorship-network/tickets-message.tspackages/workers/jam-network/messages.tspackages/workers/jam-network/protocol.ts
| const block = reencodeAsView(Block.Codec, emptyBlock(), tinyChainSpec); | ||
| await worker.offline.submitBlock(block); | ||
| assert.strictEqual(importedBlocks[0], block); | ||
|
|
||
| // Outgoing online-network announcements are observable programmatically. | ||
| let announcedHeader: unknown = null; | ||
| worker.offline.announcedHeaders.once((header) => { | ||
| announcedHeader = header; | ||
| }); | ||
| const header = WithHash.new(Bytes.zero(HASH_SIZE).asOpaque<HeaderHash>(), block.header.view()); | ||
| await worker.network.sendNewHeader(header); | ||
| assert.strictEqual(announcedHeader, header); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the README example self-contained.
Lines [125] and [134] call assert.strictEqual, but this snippet never imports or defines assert, and browsers do not provide it globally. Use explicit checks or add an environment-appropriate assertion import.
Proposed browser-safe replacement
-assert.strictEqual(importedBlocks[0], block);
+if (importedBlocks[0] !== block) {
+ throw new Error("Offline networking did not deliver the block");
+}
...
-assert.strictEqual(announcedHeader, header);
+if (announcedHeader !== header) {
+ throw new Error("Header announcement was not observed");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const block = reencodeAsView(Block.Codec, emptyBlock(), tinyChainSpec); | |
| await worker.offline.submitBlock(block); | |
| assert.strictEqual(importedBlocks[0], block); | |
| // Outgoing online-network announcements are observable programmatically. | |
| let announcedHeader: unknown = null; | |
| worker.offline.announcedHeaders.once((header) => { | |
| announcedHeader = header; | |
| }); | |
| const header = WithHash.new(Bytes.zero(HASH_SIZE).asOpaque<HeaderHash>(), block.header.view()); | |
| await worker.network.sendNewHeader(header); | |
| assert.strictEqual(announcedHeader, header); | |
| const block = reencodeAsView(Block.Codec, emptyBlock(), tinyChainSpec); | |
| await worker.offline.submitBlock(block); | |
| if (importedBlocks[0] !== block) { | |
| throw new Error("Offline networking did not deliver the block"); | |
| } | |
| // Outgoing online-network announcements are observable programmatically. | |
| let announcedHeader: unknown = null; | |
| worker.offline.announcedHeaders.once((header) => { | |
| announcedHeader = header; | |
| }); | |
| const header = WithHash.new(Bytes.zero(HASH_SIZE).asOpaque<HeaderHash>(), block.header.view()); | |
| await worker.network.sendNewHeader(header); | |
| if (announcedHeader !== header) { | |
| throw new Error("Header announcement was not observed"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@bin/lib/README.md` around lines 123 - 134, Make the README example
self-contained by replacing the two assert.strictEqual calls around
importedBlocks and announcedHeader with explicit browser-safe equality checks,
or define an appropriate assertion dependency within the example. Preserve both
validations: each value must equal the expected block or header.
| import assert from "node:assert"; | ||
| import { describe, it } from "node:test"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep this shared test runtime-neutral.
node:assert and node:test make browser.test.ts Node-only. Use the repository’s cross-runtime test/assertion utilities, or split this into explicit Node and web test files.
As per path instructions, *.test.ts files must remain compatible with both Node and web unless they use an explicit *.node.ts / *.web.ts convention.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/crypto/browser.test.ts` around lines 1 - 2, Replace the
node:assert and node:test imports in browser.test.ts with the repository’s
runtime-neutral test and assertion utilities. Keep the test compatible with both
Node and web runtimes without renaming it or introducing separate
runtime-specific files.
Source: Path instructions
| `@typeberry/crypto/browser.js` is the dependency-light entry point for code | ||
| which only needs encoded key/signature sizes and their opaque TypeScript types. | ||
| It has no runtime imports and does not expose cryptographic operations. Its | ||
| constants intentionally mirror the canonical values in `ed25519.ts` and | ||
| `bandersnatch.ts`; their literal type annotations and unit test catch drift. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the encoded-size wording.
Replace “only needs encoded key/signature sizes” with “only needs the sizes of encoded keys/signatures” to avoid ambiguity.
🧰 Tools
🪛 LanguageTool
[style] ~7-~7: The double modal “needs encoded” is nonstandard (only accepted in certain dialects). Consider “to be encoded”.
Context: ...t entry point for code which only needs encoded key/signature sizes and their opaque Ty...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/crypto/README.md` around lines 6 - 10, Update the description
of the `@typeberry/crypto/browser.js` entry point to say it is for code that only
needs the sizes of encoded keys/signatures, leaving the surrounding
documentation unchanged.
Source: Linters/SAST tools
| import assert from "node:assert"; | ||
| import { describe, it } from "node:test"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Make the new generic test files runtime-neutral.
These *.test.ts files import Node-only node:test and node:assert, so they cannot run in the required web environment. Use the repository’s browser-compatible test/assertion pattern, or explicitly designate each test as Node-only if that is intentional.
packages/core/listener/index.test.ts#L1-L2: replace Node-only test/assertion imports or rename to the explicit Node-only convention.packages/jam/networking-offline/index.test.ts#L1-L2: replace Node-only test/assertion imports or rename to the explicit Node-only convention.bin/lib/examples/networking-offline.test.ts#L1-L2: replace Node-only test/assertion imports or rename to the explicit Node-only convention.
As per path instructions, “Treat *.test.ts as test files and keep them compatible with both Node and web unless the file has an explicit *.node.ts / *.web.ts convention.”
📍 Affects 3 files
packages/core/listener/index.test.ts#L1-L2(this comment)packages/jam/networking-offline/index.test.ts#L1-L2bin/lib/examples/networking-offline.test.ts#L1-L2
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/listener/index.test.ts` around lines 1 - 2, Make the test files
runtime-neutral by replacing the Node-only imports in
packages/core/listener/index.test.ts (lines 1-2),
packages/jam/networking-offline/index.test.ts (lines 1-2), and
bin/lib/examples/networking-offline.test.ts (lines 1-2) with the repository’s
browser-compatible test and assertion utilities. Keep each file named *.test.ts
so it remains compatible with both Node and web environments; do not rename them
to a Node-only convention.
Source: Path instructions
| private assertActive(): void { | ||
| if (this.isFinished) { | ||
| throw new Error("Offline networking has finished"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Return a tagged lifecycle result instead of throwing.
submitBlock, submitBlocks, and submitTickets expose the finished-state failure as a rejected promise with a message-specific Error; the test also binds callers to that exception contract. Model the inactive state with the repository’s Result/tagged-union convention instead.
As per path instructions, “Use Result/tagged unions for error handling; avoid relying on specific thrown exceptions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/jam/networking-offline/index.ts` around lines 88 - 90, Replace
assertActive’s exception-based inactive-state handling with the repository’s
Result/tagged-union convention, and update submitBlock, submitBlocks, and
submitTickets to return the tagged lifecycle failure through their promise
results. Adjust the related test to assert the Result variant rather than a
message-specific rejected Error, while preserving successful active-state
behavior.
Source: Path instructions
1 participant
No description provided.