Use static builders instead of constructors#945
Merged
Conversation
Member
tomusdrw
commented
Apr 16, 2026
tomusdrw
commented
- Use builder methods for HostCallRegisters
- Refactor classes to use static builder methods.
- Convert more classes to use static builder
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis changeset systematically replaces public constructors with controlled factory entry points across many modules. Most classes gain static factory methods (commonly Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
tomusdrw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/networking/quic-peer.ts (1)
24-47: 🛠️ Refactor suggestion | 🟠 MajorMove side-effect setup out of the constructor.
The constructor still performs logging and event wiring, so it is not data-initialization-only. Move this setup into
QuicPeer.new(...)(or an init method called by it) to fully match the factory pattern.Suggested refactor
export class QuicPeer implements Peer { @@ static new(conn: QUICConnection, peerInfo: PeerInfo) { - return new QuicPeer(conn, peerInfo); + const peer = new QuicPeer(conn, peerInfo); + logger.log`👥 [${peerInfo.id}] peer connected ${conn.remoteHost}:${conn.remotePort}`; + peer.bindConnectionEvents(); + return peer; } private constructor( public readonly conn: QUICConnection, peerInfo: PeerInfo, ) { - logger.log`👥 [${peerInfo.id}] peer connected ${conn.remoteHost}:${conn.remotePort}`; - this.connectionId = conn.connectionIdShared.toString(); @@ this.id = peerInfo.id; this.key = peerInfo.key; + } - addEventListener(conn, events.EventQUICConnectionStream, (ev) => { + private bindConnectionEvents(): void { + addEventListener(this.conn, events.EventQUICConnectionStream, (ev) => { const stream = ev.detail; logger.log`🚰 [${this.id}] new stream: [${stream.streamId}]`; this.streamEvents.emit("stream", QuicStream.new(stream)); }); - addEventListener(conn, events.EventQUICConnectionError, (err) => { + addEventListener(this.conn, events.EventQUICConnectionError, (err) => { logger.error`❌ [${this.id}] connection failed: ${err.detail}`; }); }As per coding guidelines, constructors should be “data initialization only” with no instantiation control flow; move any creation/validation logic into the builder/factory.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/networking/quic-peer.ts` around lines 24 - 47, The constructor currently performs side effects (logging and event wiring using logger.log/logger.error and addEventListener for events.EventQUICConnectionStream and events.EventQUICConnectionError, and emitting to this.streamEvents), so refactor by moving all non-field initialization into a new init method (e.g., QuicPeer.init or performSetup) and call that from the factory QuicPeer.new(...); keep the constructor as data-initialization-only (assigning this.conn, this.connectionId, this.address, this.id, this.key), then implement the init method to attach the addEventListener handlers (wrapping stream -> this.streamEvents.emit("stream", QuicStream.new(stream))) and the logging, and ensure QuicPeer.new invokes the init method before returning the instance so behavior is unchanged.packages/jam/state/in-memory-state.ts (1)
141-148:⚠️ Potential issue | 🟡 Minor
clone()is still shallow despite the no-shared-references contract.Lines 143-147 duplicate the containers, but the contained
PreimageItem,StorageItem, andLookupHistoryItemobjects are still reused. Either deep-copy those nested items too, or relax the docstring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/state/in-memory-state.ts` around lines 141 - 148, The current clone path in InMemoryService.new uses HashDictionary.fromEntries and new Map to copy containers but leaves nested objects (values in this.data.preimages, this.data.storage and the entries inside this.data.lookupHistory) as shared references; update the clone to deep-copy those nested PreimageItem, StorageItem and LookupHistoryItem values (e.g., map over this.data.preimages.entries() and this.data.storage.entries() and produce new value objects, and for lookupHistory map and clone each HistoryItem inside each array) instead of reusing them — you can use structuredClone or each domain type’s own copy/constructor/`create` helper where available; keep ServiceAccountInfo.create and the outer container copies (HashDictionary.fromEntries / new Map) but ensure the contained items are newly created.
🧹 Nitpick comments (8)
packages/jam/jamnp-s/stream-manager.ts (1)
139-163: Use the raw stream id string when composingStreamId.At Line 140–141, converting
stream.streamIdthroughtryAsU32is unnecessary for this identifier path and can make opaqueStreamIdhandling brittle. Prefer composing with the original string value directly.Suggested change
- const quicStreamId = tryAsU32(stream.streamId); - const streamId = tryAsStreamId(`${peer.id}:${quicStreamId}`); + const streamId = tryAsStreamId(`${peer.id}:${stream.streamId}`);Based on learnings: in
packages/jam/jamnp-s/stream-manager.ts, composite StreamId values should usestream.streamIddirectly as a string, without U32 conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/jamnp-s/stream-manager.ts` around lines 139 - 163, The code currently converts stream.streamId via tryAsU32 before composing the StreamId which is brittle; in registerStream replace the two-line creation of quicStreamId and streamId (which uses tryAsU32) with a single use of the raw stream.streamId string when creating the StreamId (e.g., call tryAsStreamId with `${peer.id}:${stream.streamId}`), remove the tryAsU32 usage and any dependent quicStreamId variable, and ensure QuicStreamSender.new and any map keys (this.streams / this.backgroundTasks) continue to use the new streamId value.packages/jam/transition/hasher.ts (1)
10-15: Consider delegatingcreate()tonew(...)to keep a single construction path.This avoids duplicate constructor wiring and keeps factory maintenance tighter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/hasher.ts` around lines 10 - 15, Change TransitionHasher.create to call TransitionHasher.new so there is a single construction path: obtain the two async hashers (keccak.KeccakHasher.create() and Blake2b.createHasher()) then pass them into TransitionHasher.new(keccakHasher, blake2b) instead of calling the constructor directly. Update the static create method to await both factories, call TransitionHasher.new with the resulting keccakHasher and blake2b, and return that result, keeping the existing static new(keccakHasher, blake2b) and constructor intact.packages/core/hash/hash.ts (1)
42-45: Constructor visibility is still broader than the factory-only contract.Using
protectedstill permits subclass-based construction paths that bypass centralized builders. If strict instantiation control is intended, move to private constructors (or adjust inheritance structure accordingly).As per coding guidelines: "Prefer static new(...)/*.new style factory methods and keep the actual constructor private."
Also applies to: 58-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/hash/hash.ts` around lines 42 - 45, The constructor(s) in this file (the Hash class constructors declared as "protected constructor" around the current Hash/THash/TData declarations and the second constructor at the later block) must be made private to enforce factory-only instantiation; change both protected constructors to private and ensure any existing factory functions (e.g., static new / .new methods or factory functions that create instances of the Hash class) are used/updated to construct instances instead of relying on subclassing; if any subclass currently calls super(...), refactor that subclass to use a provided static factory on Hash or adjust the inheritance to avoid direct construction.packages/jam/config/chain-spec.ts (1)
85-91: Prefer a dedicatedChainSpecArgstype overOmit<ChainSpec, ...>.
Omit<ChainSpec, ...>couples constructor input to the full instance type. Defining an explicit args type will keep the factory contract stable and easier to reason about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/config/chain-spec.ts` around lines 85 - 91, Introduce a dedicated type alias (e.g. ChainSpecArgs) defined as the current Omit<ChainSpec, "validatorsSuperMajority" | "thirdOfValidators" | "erasureCodedPieceSize"> and replace the inline Omit usage in both the static new method signature and the private constructor signature with ChainSpecArgs; update any references to the constructor/new to use the new alias so the factory contract no longer depends directly on the full ChainSpec shape.packages/jam/transition/chain-stf.ts (1)
137-157: Move collaborator wiring from constructor intoassemble(...).
assemble(...)currently just forwards arguments, while instantiation control flow still lives in the constructor. Consider making the constructor assignment-only and doing object wiring in the static factory.As per coding guidelines, constructors should be “data initialization only” (assign fields) and instantiation logic should be moved into static builders/factories.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/chain-stf.ts` around lines 137 - 157, The constructor of OnChain should be reduced to plain data initialization (assigning chainSpec, state, hasher, options, headerChain) and any collaborator wiring currently inside the constructor must be moved into the static assemble(...) factory; change assemble(...) to instantiate the object (new OnChain(...)) and then perform all setup/wiring (registering listeners, hooking headerChain, initializing dependent services, etc.) on that instance before returning it, and remove those wiring steps from the OnChain constructor so it only assigns fields (referencing assemble, OnChain constructor, and fields chainSpec, state, hasher, options, headerChain).packages/jam/transition/reports/test.utils.ts (1)
329-350: Reuse the existingidvariable for service construction.Tiny readability cleanup: avoid recomputing
tryAsServiceId(129)whenidalready exists in scope.Proposed diff
- InMemoryService.new(tryAsServiceId(129), { + InMemoryService.new(id, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/reports/test.utils.ts` around lines 329 - 350, Replace the duplicated tryAsServiceId(129) call with the existing id variable so the InMemoryService.new invocation uses id (the pre-existing service identifier) instead of recomputing tryAsServiceId(129); update the constructor call where InMemoryService.new(...) is invoked and ensure any surrounding references (preimages, storage, info) remain unchanged and still match the id usage.packages/jam/transition/externalities/accumulate-externalities.test.ts (1)
71-78: Extract a test helper forAccumulateExternalities.forServicesetup.The same object literal is duplicated many times; a local helper would cut boilerplate and keep defaults consistent.
♻️ Suggested refactor pattern
+function forServiceExt(state: PartiallyUpdatedState, currentServiceId = tryAsServiceId(0), currentTimeslot = tryAsTimeSlot(16)) { + return AccumulateExternalities.forService({ + chainSpec: tinyChainSpec, + blake2b, + updatedState: state, + currentServiceId, + nextNewServiceIdCandidate: tryAsServiceId(10), + currentTimeslot, + }); +} ... -const partialState = AccumulateExternalities.forService({ - chainSpec: tinyChainSpec, - blake2b: blake2b, - updatedState: state, - currentServiceId: tryAsServiceId(0), - nextNewServiceIdCandidate: tryAsServiceId(10), - currentTimeslot: tryAsTimeSlot(16), -}); +const partialState = forServiceExt(state);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/externalities/accumulate-externalities.test.ts` around lines 71 - 78, Extract a test helper function (e.g., makeAccumulateExternalitiesArgs or buildForServiceOpts) that returns the common argument object used for AccumulateExternalities.forService so tests reuse defaults (tinyChainSpec, blake2b, state, tryAsServiceId(0), tryAsServiceId(10), tryAsTimeSlot(16)); update each test to call AccumulateExternalities.forService(helper()) and override only fields that differ per test (e.g., currentServiceId or updatedState) to remove duplicated object literals and keep defaults consistent across tests.packages/jam/transition/externalities/accumulate-externalities.ts (1)
96-102: Prefer returningResultfromforServicevalidation instead of throwing.Line 99 introduces exception-based control flow for an expected validation failure. Consider making
forServicereturnResultso callers can handle this path explicitly.♻️ Proposed refactor
- static forService(args: AccumulateExternalitiesArgs) { + static forService(args: AccumulateExternalitiesArgs): Result<AccumulateExternalities, "missing_service_info"> { const service = args.updatedState.getServiceInfo(args.currentServiceId); if (service === null) { - throw new Error(`Invalid state initialization. Service info missing for ${args.currentServiceId}.`); + return Result.error( + "missing_service_info", + () => `Invalid state initialization. Service info missing for ${args.currentServiceId}.`, + ); } - return new AccumulateExternalities(args); + return Result.ok(new AccumulateExternalities(args)); }As per coding guidelines: "Prefer Result over throwing; avoid relying on catching specific exceptions."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/externalities/accumulate-externalities.ts` around lines 96 - 102, forService currently throws when getServiceInfo returns null; change its signature to return a Result<AccumulateExternalities, ValidationError> (or your project's Result type) instead of throwing: call args.updatedState.getServiceInfo(args.currentServiceId), and if null return Err with a descriptive validation error referencing args.currentServiceId, otherwise return Ok(new AccumulateExternalities(args)); update the function signature and any callers of forService to handle the Result rather than relying on exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/collections/multi-map.ts`:
- Around line 31-37: Move the instantiation validation out of the private
constructor and into the static factory new<TKeys extends readonly unknown[],
TValue>(keysLength: TKeys["length"], keyMappers?: KeyMappers<TKeys>) method:
remove the check`${keysLength > 0} Keys cannot be empty.` and check`${keyMappers
=== undefined || keyMappers.length === keysLength} Incorrect number of key
mappers given!` calls from the MultiMap constructor and add those checks at the
start of MultiMap.new before calling new MultiMap(...), keeping the constructor
strictly data-initialization only (assign fields).
In `@packages/core/pvm-interpreter/program-decoder/jump-table.ts`:
- Line 15: Update the validation error message in jump-table.ts to fix the typo
"item lenght" to "item length" so diagnostics read "Length of jump table
(${bytes.length}) should be a multiple of item length (${itemByteLength})!";
locate the string in the code that constructs this message (around the jump
table length validation that references bytes.length and itemByteLength) and
replace the misspelled word only.
- Around line 12-21: The fromRaw validation currently allows itemByteLength ===
0 with non-empty bytes and then the JumpTable constructor forces length to 0,
silently dropping data; update fromRaw in class JumpTable to reject non-empty
bytes when itemByteLength is 0 by asserting bytes.length === 0 in that branch
(i.e. require either itemByteLength > 0 and bytes.length % itemByteLength === 0,
or itemByteLength === 0 and bytes.length === 0) and keep a clear error message
referencing bytes.length and itemByteLength so malformed input is surfaced
instead of being discarded; adjust the check surrounding fromRaw and leave the
private constructor (JumpTable) behavior unchanged.
In `@packages/core/pvm-interpreter/registers.ts`:
- Around line 23-30: The fromBytes(bytes: Uint8Array) method must validate
8-byte alignment before creating BigInt64Array/BigUint64Array views to avoid a
RangeError; add a check that bytes.byteOffset % 8 === 0 (in addition to the
existing size check) and throw a clear error (e.g., via the same check`...`
helper) if misaligned, so that Registers constructor and the creation of
this.asSigned / this.asUnsigned only run on properly aligned buffers.
In `@packages/core/trie/nodesDb.ts`:
- Around line 62-65: WriteableNodesDb must declare an explicit private
constructor to call the protected parent constructor and enforce factory-only
creation; add a private constructor in class WriteableNodesDb that accepts
(hasher: TrieHasher) and calls super(hasher), then keep the existing static
new(hasher: TrieHasher) factory as the only public creation point to satisfy
TypeScript and the project's guideline of private constructors with static
builders.
In `@packages/jam/jamnp-s/protocol/ce-133-work-package-submission.ts`:
- Around line 77-79: ClientHandler currently exposes an implicit public
constructor so calling new ClientHandler() bypasses the static factory; make the
constructor explicit and private (add a private constructor method to the
ClientHandler class) so instances can only be created via the existing static
new() factory method — locate the ClientHandler class and add a private
constructor definition to enforce the factory-only pattern used by
ServerHandler.
In `@packages/jam/jamnp-s/protocol/ce-134-work-package-sharing.ts`:
- Around line 127-129: ClientHandler currently exposes public construction via
the implicit constructor even though it provides a static new() factory; add an
explicit private constructor to ClientHandler (matching the pattern used by
ServerHandler) so external code cannot call new ClientHandler() directly,
leaving the static new() method as the only public instantiation path.
In `@packages/jam/state/in-memory-state.ts`:
- Around line 91-100: The constructor of InMemoryService is currently protected
which allows subclass instantiation; change the constructor visibility to
private so instances can only be created via the static new(serviceId:
ServiceId, data: InMemoryServiceData) factory; update the constructor
declaration in the InMemoryService class (the constructor handling readonly
serviceId and data) to be private and ensure no internal subclasses rely on
protected access (adjust or remove subclassing/tests if present).
---
Outside diff comments:
In `@packages/core/networking/quic-peer.ts`:
- Around line 24-47: The constructor currently performs side effects (logging
and event wiring using logger.log/logger.error and addEventListener for
events.EventQUICConnectionStream and events.EventQUICConnectionError, and
emitting to this.streamEvents), so refactor by moving all non-field
initialization into a new init method (e.g., QuicPeer.init or performSetup) and
call that from the factory QuicPeer.new(...); keep the constructor as
data-initialization-only (assigning this.conn, this.connectionId, this.address,
this.id, this.key), then implement the init method to attach the
addEventListener handlers (wrapping stream -> this.streamEvents.emit("stream",
QuicStream.new(stream))) and the logging, and ensure QuicPeer.new invokes the
init method before returning the instance so behavior is unchanged.
In `@packages/jam/state/in-memory-state.ts`:
- Around line 141-148: The current clone path in InMemoryService.new uses
HashDictionary.fromEntries and new Map to copy containers but leaves nested
objects (values in this.data.preimages, this.data.storage and the entries inside
this.data.lookupHistory) as shared references; update the clone to deep-copy
those nested PreimageItem, StorageItem and LookupHistoryItem values (e.g., map
over this.data.preimages.entries() and this.data.storage.entries() and produce
new value objects, and for lookupHistory map and clone each HistoryItem inside
each array) instead of reusing them — you can use structuredClone or each domain
type’s own copy/constructor/`create` helper where available; keep
ServiceAccountInfo.create and the outer container copies
(HashDictionary.fromEntries / new Map) but ensure the contained items are newly
created.
---
Nitpick comments:
In `@packages/core/hash/hash.ts`:
- Around line 42-45: The constructor(s) in this file (the Hash class
constructors declared as "protected constructor" around the current
Hash/THash/TData declarations and the second constructor at the later block)
must be made private to enforce factory-only instantiation; change both
protected constructors to private and ensure any existing factory functions
(e.g., static new / .new methods or factory functions that create instances of
the Hash class) are used/updated to construct instances instead of relying on
subclassing; if any subclass currently calls super(...), refactor that subclass
to use a provided static factory on Hash or adjust the inheritance to avoid
direct construction.
In `@packages/jam/config/chain-spec.ts`:
- Around line 85-91: Introduce a dedicated type alias (e.g. ChainSpecArgs)
defined as the current Omit<ChainSpec, "validatorsSuperMajority" |
"thirdOfValidators" | "erasureCodedPieceSize"> and replace the inline Omit usage
in both the static new method signature and the private constructor signature
with ChainSpecArgs; update any references to the constructor/new to use the new
alias so the factory contract no longer depends directly on the full ChainSpec
shape.
In `@packages/jam/jamnp-s/stream-manager.ts`:
- Around line 139-163: The code currently converts stream.streamId via tryAsU32
before composing the StreamId which is brittle; in registerStream replace the
two-line creation of quicStreamId and streamId (which uses tryAsU32) with a
single use of the raw stream.streamId string when creating the StreamId (e.g.,
call tryAsStreamId with `${peer.id}:${stream.streamId}`), remove the tryAsU32
usage and any dependent quicStreamId variable, and ensure QuicStreamSender.new
and any map keys (this.streams / this.backgroundTasks) continue to use the new
streamId value.
In `@packages/jam/transition/chain-stf.ts`:
- Around line 137-157: The constructor of OnChain should be reduced to plain
data initialization (assigning chainSpec, state, hasher, options, headerChain)
and any collaborator wiring currently inside the constructor must be moved into
the static assemble(...) factory; change assemble(...) to instantiate the object
(new OnChain(...)) and then perform all setup/wiring (registering listeners,
hooking headerChain, initializing dependent services, etc.) on that instance
before returning it, and remove those wiring steps from the OnChain constructor
so it only assigns fields (referencing assemble, OnChain constructor, and fields
chainSpec, state, hasher, options, headerChain).
In `@packages/jam/transition/externalities/accumulate-externalities.test.ts`:
- Around line 71-78: Extract a test helper function (e.g.,
makeAccumulateExternalitiesArgs or buildForServiceOpts) that returns the common
argument object used for AccumulateExternalities.forService so tests reuse
defaults (tinyChainSpec, blake2b, state, tryAsServiceId(0), tryAsServiceId(10),
tryAsTimeSlot(16)); update each test to call
AccumulateExternalities.forService(helper()) and override only fields that
differ per test (e.g., currentServiceId or updatedState) to remove duplicated
object literals and keep defaults consistent across tests.
In `@packages/jam/transition/externalities/accumulate-externalities.ts`:
- Around line 96-102: forService currently throws when getServiceInfo returns
null; change its signature to return a Result<AccumulateExternalities,
ValidationError> (or your project's Result type) instead of throwing: call
args.updatedState.getServiceInfo(args.currentServiceId), and if null return Err
with a descriptive validation error referencing args.currentServiceId, otherwise
return Ok(new AccumulateExternalities(args)); update the function signature and
any callers of forService to handle the Result rather than relying on
exceptions.
In `@packages/jam/transition/hasher.ts`:
- Around line 10-15: Change TransitionHasher.create to call TransitionHasher.new
so there is a single construction path: obtain the two async hashers
(keccak.KeccakHasher.create() and Blake2b.createHasher()) then pass them into
TransitionHasher.new(keccakHasher, blake2b) instead of calling the constructor
directly. Update the static create method to await both factories, call
TransitionHasher.new with the resulting keccakHasher and blake2b, and return
that result, keeping the existing static new(keccakHasher, blake2b) and
constructor intact.
In `@packages/jam/transition/reports/test.utils.ts`:
- Around line 329-350: Replace the duplicated tryAsServiceId(129) call with the
existing id variable so the InMemoryService.new invocation uses id (the
pre-existing service identifier) instead of recomputing tryAsServiceId(129);
update the constructor call where InMemoryService.new(...) is invoked and ensure
any surrounding references (preimages, storage, info) remain unchanged and still
match the id usage.
🪄 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: 95fad167-92fc-462c-8961-6defcc577f33
📒 Files selected for processing (248)
bin/ipc2rpc/client.tsbin/jam/index.tsbin/lib/examples/pvm-usage.test.tsbin/pvm/index.tsbin/rpc/main.tsbin/rpc/src/handlers/typeberry/refine-work-package.tsbin/rpc/src/server.tsbin/rpc/src/subscription-manager.tsbin/rpc/test/e2e.tsbin/test-runner/common.tsbin/test-runner/index.tsbin/test-runner/reporter.tsbin/test-runner/state-transition/state-transition.tsbin/test-runner/w3f/preimages.tsbin/test-runner/w3f/pvm-gas-cost.tsbin/test-runner/w3f/pvm.tspackages/core/codec/descriptor.tspackages/core/codec/descriptors.tspackages/core/codec/index.test.tspackages/core/codec/skip.tspackages/core/codec/view.tspackages/core/collections/multi-map.test.tspackages/core/collections/multi-map.tspackages/core/concurrent/parent.tspackages/core/crypto/ed25519.tspackages/core/hash/hash.tspackages/core/networking/quic-network.tspackages/core/networking/quic-peer.tspackages/core/networking/quic-stream.tspackages/core/networking/setup.tspackages/core/pvm-host-calls/bin.tspackages/core/pvm-host-calls/ecalli-trace-logger.test.tspackages/core/pvm-host-calls/host-call-memory.test.tspackages/core/pvm-host-calls/host-call-memory.tspackages/core/pvm-host-calls/host-call-registers.test.tspackages/core/pvm-host-calls/host-call-registers.tspackages/core/pvm-host-calls/host-calls-executor.tspackages/core/pvm-host-calls/host-calls.tspackages/core/pvm-host-calls/pvm-instance-manager.tspackages/core/pvm-interpreter-ananas/index.tspackages/core/pvm-interpreter/args-decoder/args-decoder.test.tspackages/core/pvm-interpreter/args-decoder/args-decoder.tspackages/core/pvm-interpreter/args-decoder/args-decoding-results.tspackages/core/pvm-interpreter/args-decoder/decoders/extended-with-immediate-decoder.test.tspackages/core/pvm-interpreter/args-decoder/decoders/extended-with-immediate-decoder.tspackages/core/pvm-interpreter/args-decoder/decoders/immediate-decoder.test.tspackages/core/pvm-interpreter/args-decoder/decoders/immediate-decoder.tspackages/core/pvm-interpreter/basic-blocks/basic-blocks.test.tspackages/core/pvm-interpreter/bin.tspackages/core/pvm-interpreter/debugger-adapter.tspackages/core/pvm-interpreter/gas.tspackages/core/pvm-interpreter/interpreter.tspackages/core/pvm-interpreter/memory/memory-builder.test.tspackages/core/pvm-interpreter/memory/memory-builder.tspackages/core/pvm-interpreter/memory/memory.test.tspackages/core/pvm-interpreter/memory/memory.tspackages/core/pvm-interpreter/memory/pages/readable-page.test.tspackages/core/pvm-interpreter/memory/pages/readable-page.tspackages/core/pvm-interpreter/memory/pages/writeable-page.test.tspackages/core/pvm-interpreter/memory/pages/writeable-page.tspackages/core/pvm-interpreter/ops-dispatchers/no-args-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-imm-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-offset-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-reg-one-ext-imm-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-reg-one-imm-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-reg-one-imm-one-offset-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/one-reg-two-imms-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/three-regs-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/two-imms-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/two-regs-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/two-regs-one-imm-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/two-regs-one-offset-dispatcher.test.tspackages/core/pvm-interpreter/ops-dispatchers/two-regs-two-imms-dispatcher.test.tspackages/core/pvm-interpreter/ops/bit-ops.test.tspackages/core/pvm-interpreter/ops/bit-ops.tspackages/core/pvm-interpreter/ops/bit-rotation-ops.test.tspackages/core/pvm-interpreter/ops/bit-rotation-ops.tspackages/core/pvm-interpreter/ops/boolean-ops.test.tspackages/core/pvm-interpreter/ops/boolean-ops.tspackages/core/pvm-interpreter/ops/branch-ops.test.tspackages/core/pvm-interpreter/ops/branch-ops.tspackages/core/pvm-interpreter/ops/dynamic-jump-ops.test.tspackages/core/pvm-interpreter/ops/dynamic-jump-ops.tspackages/core/pvm-interpreter/ops/host-call-ops.test.tspackages/core/pvm-interpreter/ops/host-call-ops.tspackages/core/pvm-interpreter/ops/load-ops.test.tspackages/core/pvm-interpreter/ops/load-ops.tspackages/core/pvm-interpreter/ops/math-ops.test.tspackages/core/pvm-interpreter/ops/math-ops.tspackages/core/pvm-interpreter/ops/memory-ops.test.tspackages/core/pvm-interpreter/ops/memory-ops.tspackages/core/pvm-interpreter/ops/move-ops.test.tspackages/core/pvm-interpreter/ops/move-ops.tspackages/core/pvm-interpreter/ops/no-args-ops.test.tspackages/core/pvm-interpreter/ops/no-args-ops.tspackages/core/pvm-interpreter/ops/shift-ops.test.tspackages/core/pvm-interpreter/ops/shift-ops.tspackages/core/pvm-interpreter/ops/store-ops.test.tspackages/core/pvm-interpreter/ops/store-ops.tspackages/core/pvm-interpreter/program-decoder/jump-table.test.tspackages/core/pvm-interpreter/program-decoder/jump-table.tspackages/core/pvm-interpreter/program-decoder/mask.test.tspackages/core/pvm-interpreter/program-decoder/mask.tspackages/core/pvm-interpreter/program-decoder/program-decoder.test.tspackages/core/pvm-interpreter/program-decoder/program-decoder.tspackages/core/pvm-interpreter/program.tspackages/core/pvm-interpreter/registers.test.tspackages/core/pvm-interpreter/registers.tspackages/core/pvm-interpreter/spi-decoder/decode-standard-program.tspackages/core/trie/nodes.tspackages/core/trie/nodesDb.tspackages/core/trie/trie.tspackages/extensions/ipc/index.tspackages/extensions/ipc/jamnp/handler.tspackages/extensions/ipc/jamnp/server.tspackages/extensions/ipc/server.tspackages/jam/block/header.tspackages/jam/config-node/jip-chain-spec.tspackages/jam/config/chain-spec.tspackages/jam/config/network.tspackages/jam/database-lmdb/blocks.tspackages/jam/database-lmdb/root.tspackages/jam/database-lmdb/states.test.tspackages/jam/database-lmdb/states.tspackages/jam/database/blocks.test.tspackages/jam/database/states.test.tspackages/jam/database/states.tspackages/jam/executor/pvm-executor.tspackages/jam/fuzz-proto/v1/handler.test.tspackages/jam/fuzz-proto/v1/handler.tspackages/jam/in-core/externalities/refine.test.tspackages/jam/in-core/in-core.test.tspackages/jam/in-core/in-core.tspackages/jam/in-core/is-authorized.test.tspackages/jam/in-core/refine.tspackages/jam/jam-host-calls/accumulate/assign.test.tspackages/jam/jam-host-calls/accumulate/assign.tspackages/jam/jam-host-calls/accumulate/bless.test.tspackages/jam/jam-host-calls/accumulate/bless.tspackages/jam/jam-host-calls/accumulate/checkpoint.test.tspackages/jam/jam-host-calls/accumulate/checkpoint.tspackages/jam/jam-host-calls/accumulate/designate.test.tspackages/jam/jam-host-calls/accumulate/designate.tspackages/jam/jam-host-calls/accumulate/eject.test.tspackages/jam/jam-host-calls/accumulate/eject.tspackages/jam/jam-host-calls/accumulate/forget.test.tspackages/jam/jam-host-calls/accumulate/forget.tspackages/jam/jam-host-calls/accumulate/new.test.tspackages/jam/jam-host-calls/accumulate/new.tspackages/jam/jam-host-calls/accumulate/provide.test.tspackages/jam/jam-host-calls/accumulate/provide.tspackages/jam/jam-host-calls/accumulate/query.test.tspackages/jam/jam-host-calls/accumulate/query.tspackages/jam/jam-host-calls/accumulate/solicit.test.tspackages/jam/jam-host-calls/accumulate/solicit.tspackages/jam/jam-host-calls/accumulate/transfer.test.tspackages/jam/jam-host-calls/accumulate/transfer.tspackages/jam/jam-host-calls/accumulate/upgrade.test.tspackages/jam/jam-host-calls/accumulate/upgrade.tspackages/jam/jam-host-calls/accumulate/yield.test.tspackages/jam/jam-host-calls/accumulate/yield.tspackages/jam/jam-host-calls/externalities/refine-externalities.test.tspackages/jam/jam-host-calls/externalities/state-update.tspackages/jam/jam-host-calls/externalities/test-accounts.tspackages/jam/jam-host-calls/general/fetch.test.tspackages/jam/jam-host-calls/general/fetch.tspackages/jam/jam-host-calls/general/gas.test.tspackages/jam/jam-host-calls/general/gas.tspackages/jam/jam-host-calls/general/info.test.tspackages/jam/jam-host-calls/general/info.tspackages/jam/jam-host-calls/general/log.tspackages/jam/jam-host-calls/general/lookup.test.tspackages/jam/jam-host-calls/general/lookup.tspackages/jam/jam-host-calls/general/read.test.tspackages/jam/jam-host-calls/general/read.tspackages/jam/jam-host-calls/general/write.test.tspackages/jam/jam-host-calls/general/write.tspackages/jam/jam-host-calls/refine/export.test.tspackages/jam/jam-host-calls/refine/export.tspackages/jam/jam-host-calls/refine/expunge.test.tspackages/jam/jam-host-calls/refine/expunge.tspackages/jam/jam-host-calls/refine/historical-lookup.test.tspackages/jam/jam-host-calls/refine/historical-lookup.tspackages/jam/jam-host-calls/refine/invoke.test.tspackages/jam/jam-host-calls/refine/invoke.tspackages/jam/jam-host-calls/refine/machine.test.tspackages/jam/jam-host-calls/refine/machine.tspackages/jam/jam-host-calls/refine/pages.test.tspackages/jam/jam-host-calls/refine/pages.tspackages/jam/jam-host-calls/refine/peek.test.tspackages/jam/jam-host-calls/refine/peek.tspackages/jam/jam-host-calls/refine/poke.test.tspackages/jam/jam-host-calls/refine/poke.tspackages/jam/jamnp-s/network.tspackages/jam/jamnp-s/peers.tspackages/jam/jamnp-s/protocol/ce-128-block-request.test.tspackages/jam/jamnp-s/protocol/ce-128-block-request.tspackages/jam/jamnp-s/protocol/ce-129-state-request.test.tspackages/jam/jamnp-s/protocol/ce-129-state-request.tspackages/jam/jamnp-s/protocol/ce-131-ce-132-safrole-ticket-distribution.test.tspackages/jam/jamnp-s/protocol/ce-131-ce-132-safrole-ticket-distribution.tspackages/jam/jamnp-s/protocol/ce-133-work-package-submission.test.tspackages/jam/jamnp-s/protocol/ce-133-work-package-submission.tspackages/jam/jamnp-s/protocol/ce-134-work-package-sharing.test.tspackages/jam/jamnp-s/protocol/ce-134-work-package-sharing.tspackages/jam/jamnp-s/protocol/ce-135-work-report-distribution.test.tspackages/jam/jamnp-s/protocol/ce-135-work-report-distribution.tspackages/jam/jamnp-s/protocol/up-0-block-announcement.tspackages/jam/jamnp-s/stream-manager.tspackages/jam/jamnp-s/tasks/sync.test.tspackages/jam/jamnp-s/tasks/sync.tspackages/jam/jamnp-s/tasks/ticket-distribution.test.tspackages/jam/jamnp-s/tasks/ticket-distribution.tspackages/jam/node/common.tspackages/jam/rpc-client/index.tspackages/jam/state-json/accounts.tspackages/jam/state-merkleization/in-memory-state-codec.tspackages/jam/state-merkleization/serialized-state-view.tspackages/jam/state-merkleization/serialized-state.test.tspackages/jam/state-merkleization/serialized-state.tspackages/jam/state/in-memory-state-view.tspackages/jam/state/in-memory-state.test.tspackages/jam/state/in-memory-state.tspackages/jam/state/service.tspackages/jam/state/test.utils.tspackages/jam/transition/accumulate/accumulate.test.tspackages/jam/transition/accumulate/accumulate.tspackages/jam/transition/block-verifier.test.tspackages/jam/transition/block-verifier.tspackages/jam/transition/chain-stf.tspackages/jam/transition/externalities/accumulate-externalities.test.tspackages/jam/transition/externalities/accumulate-externalities.tspackages/jam/transition/externalities/refine-fetch-externalities.test.tspackages/jam/transition/externalities/refine-fetch-externalities.tspackages/jam/transition/hasher.test.tspackages/jam/transition/hasher.tspackages/jam/transition/preimages.test.tspackages/jam/transition/reports/test.utils.tspackages/workers/api-node/config.tspackages/workers/api-node/port.test.tspackages/workers/api-node/port.tspackages/workers/api-node/protocol.tspackages/workers/block-authorship/generator.test.tspackages/workers/block-authorship/generator.tspackages/workers/block-authorship/main.tspackages/workers/importer/finality.test.tspackages/workers/importer/importer.tspackages/workers/importer/main.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bin/lib/README.md (1)
295-297:⚠️ Potential issue | 🟠 MajorUpdate remaining PVM example to static builder for consistency.
Line 296 still uses
new Interpreter()while nearby examples useInterpreter.new(). This leaves the docs inconsistent and may break if constructor access is restricted.Suggested README fix
const initialGas = tryAsGas(1000); -const pvm = new Interpreter(); +const pvm = Interpreter.new(); pvm.resetGeneric(program.raw, 0, initialGas);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/README.md` around lines 295 - 297, The README example uses the constructor directly (new Interpreter()) which is inconsistent with other examples and may fail if the constructor is restricted; update the snippet to use the static builder Interpreter.new() instead, i.e., create the PVM via Interpreter.new(...) and then call resetGeneric(program.raw, 0, initialGas) (keeping tryAsGas(1000) and resetGeneric usage intact) so the example matches other docs and uses the static factory method consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@bin/lib/README.md`:
- Around line 295-297: The README example uses the constructor directly (new
Interpreter()) which is inconsistent with other examples and may fail if the
constructor is restricted; update the snippet to use the static builder
Interpreter.new() instead, i.e., create the PVM via Interpreter.new(...) and
then call resetGeneric(program.raw, 0, initialGas) (keeping tryAsGas(1000) and
resetGeneric usage intact) so the example matches other docs and uses the static
factory method consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6d98533-a7d7-4773-8cda-77ea476bdd47
📒 Files selected for processing (3)
bin/lib/README.mdpackages/jam/in-core/externalities/refine.test.tspackages/jam/in-core/externalities/refine.ts
✅ Files skipped from review due to trivial changes (1)
- packages/jam/in-core/externalities/refine.test.ts
View all
Benchmarks summary: 83/83 OK ✅ |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.