Conversation
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR renames and adds segment-related constants (W_S → W_P; MAX_NUMBER_OF_SEGMENTS → MAX_NUMBER_OF_IMPORTS_WP; adds MAX_NUMBER_OF_EXPORTS_WP = 3072) and updates SEGMENT_BYTES to use W_P. It threads an exportOffset through the refine flow, makes RefineExternalitiesImpl record exported segments and return segment indices from exportSegment while enforcing the new export limit, exposes getExportedSegments(), and updates callers in InCore and tests. Several Gray Paper doc URLs were updated to v0.7.2. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/in-core/in-core.ts (1)
342-355:⚠️ Potential issue | 🟠 MajorDo not propagate exports when export-count validation fails.
This branch returns
exportseven onincorrectNumberOfExports. Thenrefine()incrementsexportOffsetfromresult.exports.lengthon Line 191, which can shift indices for later work items after an invalid item.Suggested fix
if (exports.length !== item.exportCount) { return { - exports, + exports: [], result: WorkResult.create({ ...baseResult, result: WorkExecResult.error(WorkExecResultKind.incorrectNumberOfExports), load: WorkRefineLoad.create({ ...baseLoad, gasUsed: tryAsServiceGas(item.refineGasLimit), exportedSegments: tryAsU32(0), }), }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/in-core.ts` around lines 342 - 355, The branch that handles export-count mismatch currently returns the original exports array which causes refine() to advance exportOffset by result.exports.length and corrupt later indices; modify the incorrectNumberOfExports branch (the block checking exports.length !== item.exportCount) to not propagate the original exports — return an empty exports array (or clear exports) alongside the WorkResult that sets exportedSegments to 0 and the error WorkExecResultKind.incorrectNumberOfExports so that result.exports.length is zero and exportOffset is not advanced; locate the check using exports and item.exportCount and update the returned object to supply an empty exports list instead of the existing exports.
🤖 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/jam/block/work-item-segment.ts`:
- Around line 29-31: The comment above the exported segment formula references a
stale symbol W_S while the implementation uses W_P; update the doc comment for
the constants (W_G, W_E, W_P) in work-item-segment.ts so the formula reads `W_G
= W_E * W_P` (or otherwise matches the actual variable names used in the file)
and ensure any explanatory text or spec link remains consistent with the W_P
name.
In `@packages/jam/in-core/externalities/refine.ts`:
- Around line 10-21: The package is importing types like SegmentExportError and
RefineExternalities from `@typeberry/jam-host-calls` but that dependency is not
declared; update packages/jam/in-core/package.json to add
`@typeberry/jam-host-calls` (appropriate semver, e.g. same workspace version or
matching version used elsewhere) under "dependencies" (or "devDependencies" if
only used in tests/build) so import/no-extraneous-dependencies is satisfied and
the imports (SegmentExportError, RefineExternalities, MachineId, etc.) resolve
correctly.
---
Outside diff comments:
In `@packages/jam/in-core/in-core.ts`:
- Around line 342-355: The branch that handles export-count mismatch currently
returns the original exports array which causes refine() to advance exportOffset
by result.exports.length and corrupt later indices; modify the
incorrectNumberOfExports branch (the block checking exports.length !==
item.exportCount) to not propagate the original exports — return an empty
exports array (or clear exports) alongside the WorkResult that sets
exportedSegments to 0 and the error WorkExecResultKind.incorrectNumberOfExports
so that result.exports.length is zero and exportOffset is not advanced; locate
the check using exports and item.exportCount and update the returned object to
supply an empty exports list instead of the existing exports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6dff36d-b98c-4796-8dd0-64bd0e040995
📒 Files selected for processing (7)
packages/jam/block/work-item-segment.tspackages/jam/block/work-item.tspackages/jam/in-core/externalities/refine.test.tspackages/jam/in-core/externalities/refine.tspackages/jam/in-core/in-core.tspackages/jam/jam-host-calls/refine/export.tspackages/jam/jam-host-calls/refine/historical-lookup.ts
tomusdrw
reviewed
Mar 17, 2026
Co-authored-by: Tomek Drwięga <tomusdrw@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jam/in-core/externalities/refine.ts (1)
61-67: Consider returning a defensive copy.
getExportedSegments()returns the internal array directly, allowing callers to mutate it. If this is intentional for performance, consider adding a comment noting that callers must not mutate the returned array. Otherwise, return a shallow copy with[...this.exportedSegments].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/externalities/refine.ts` around lines 61 - 67, getExportedSegments currently returns the internal array exportedSegments directly which allows external mutation; change getExportedSegments() to return a shallow copy (e.g., return [...this.exportedSegments]) so callers cannot mutate internal state, and update the method comment to reflect that a copy is returned; reference the getExportedSegments method and the exportedSegments property when making the change.
🤖 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/jam/in-core/externalities/refine.test.ts`:
- Around line 191-203: The inline comment in the test incorrectly mentions
MAX_NUMBER_OF_SEGMENTS; update the comment to reference the correct constant
MAX_NUMBER_OF_EXPORTS_WP so it matches the assertion and setup. Locate the test
block that calls createExt({ exportOffset: MAX_NUMBER_OF_EXPORTS_WP - 1 }) and
exportSegment(createSegment(...)) and change the comment that currently reads
about MAX_NUMBER_OF_SEGMENTS to mention MAX_NUMBER_OF_EXPORTS_WP (or rephrase to
“index = MAX_NUMBER_OF_EXPORTS_WP - 1”) so the comment accurately reflects the
code.
---
Nitpick comments:
In `@packages/jam/in-core/externalities/refine.ts`:
- Around line 61-67: getExportedSegments currently returns the internal array
exportedSegments directly which allows external mutation; change
getExportedSegments() to return a shallow copy (e.g., return
[...this.exportedSegments]) so callers cannot mutate internal state, and update
the method comment to reflect that a copy is returned; reference the
getExportedSegments method and the exportedSegments property when making the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cda480d4-13bb-41c3-80cd-5909f340fa86
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
packages/jam/block/work-item-segment.tspackages/jam/in-core/externalities/refine.test.tspackages/jam/in-core/externalities/refine.tspackages/jam/in-core/package.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/jam/in-core/externalities/refine.ts`:
- Around line 61-62: getExportedSegments currently returns the internal mutable
array exportedSegments by reference; change it to return a copy (e.g., create
and return a shallow copy of exportedSegments) so callers cannot mutate internal
state, and update the method signature to a readonly array (e.g., readonly
Segment[] in the interface) to tighten the contract and prevent mutation by
consumers; locate getExportedSegments and exportedSegments in the class and
apply these changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2355b79-b05f-4399-b9a5-f4b341875692
📒 Files selected for processing (3)
packages/jam/in-core/externalities/refine.tspackages/jam/in-core/in-core.tspackages/jam/jam-host-calls/externalities/refine-externalities.ts
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 (2)
packages/jam/jam-host-calls/externalities/refine-externalities.test.ts (1)
155-161:⚠️ Potential issue | 🟡 MinorTrack exported segments only on successful exports.
Line 160 appends to
exportSegmentseven whenexportSegmentreturns an error. This makes the test double diverge from real refine externalities behavior and can skew failure-path assertions.Suggested fix
exportSegment(segment: Segment): Result<SegmentIndex, SegmentExportError> { const result = this.exportSegmentData.get(segment); if (result === undefined) { throw new Error(`Unexpected call to exportSegment with: ${segment}`); } - this.exportSegments.push(segment); + if (result.isOk) { + this.exportSegments.push(segment); + } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/jam-host-calls/externalities/refine-externalities.test.ts` around lines 155 - 161, The test helper's exportSegment currently pushes the segment into exportSegments regardless of the Result in exportSegmentData; change exportSegment (the method) to inspect the Result returned from exportSegmentData.get(segment) and only push to exportSegments when the Result indicates a successful export (i.e., the Ok/Success variant), leaving error results untracked; use the existing exportSegmentData and exportSegments fields to perform this conditional push so failure-path assertions match real refine externalities behavior.packages/jam/in-core/in-core.ts (1)
342-353:⚠️ Potential issue | 🟠 MajorKeep
load.exportedSegmentsconsistent with returnedexportson mismatch path.When Line 342 fails, this branch still returns
exports, but Line 351 reportsexportedSegments: 0. That creates inconsistent report data for the same item.Suggested fix
if (exports.length !== item.exportCount) { return { exports, result: WorkResult.create({ ...baseResult, result: WorkExecResult.error(WorkExecResultKind.incorrectNumberOfExports), load: WorkRefineLoad.create({ ...baseLoad, gasUsed: tryAsServiceGas(item.refineGasLimit), - exportedSegments: tryAsU32(0), + exportedSegments: tryAsU32(exports.length), }), }), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/in-core.ts` around lines 342 - 353, The mismatch branch in in-core.ts returns the variable exports but sets load.exportedSegments to 0; update the incorrect-number-of-exports branch so the returned WorkRefineLoad.exportedSegments reflects the actual exports count (use exports.length via tryAsU32(exports.length) instead of tryAsU32(0)), keeping the rest of the error result and gas logic the same; locate this change in the block that constructs WorkResult.create when exports.length !== item.exportCount.
🧹 Nitpick comments (1)
packages/jam/in-core/in-core.ts (1)
36-44: Consider aligningRefineResult.exportswith readonly semantics.
RefineItemResult.exportsis readonly, butRefineResult.exportsis still mutable (PerWorkItem<Segment[]>). Keeping both readonly would make the API contract consistent end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/in-core.ts` around lines 36 - 44, RefineResult.exports is currently mutable while RefineItemResult.exports is readonly; update the RefineResult type so its exports use the same readonly semantics by changing the exports generic from PerWorkItem<Segment[]> to PerWorkItem<readonly Segment[]> (or an equivalent readonly Segment[] type) so both RefineResult.exports and RefineItemResult.exports are consistent; adjust any consuming code or type assertions that assume mutability of RefineResult.exports (look for usages of RefineResult and PerWorkItem) and run type checks to ensure no breaks.
🤖 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 `@packages/jam/in-core/in-core.ts`:
- Around line 342-353: The mismatch branch in in-core.ts returns the variable
exports but sets load.exportedSegments to 0; update the
incorrect-number-of-exports branch so the returned
WorkRefineLoad.exportedSegments reflects the actual exports count (use
exports.length via tryAsU32(exports.length) instead of tryAsU32(0)), keeping the
rest of the error result and gas logic the same; locate this change in the block
that constructs WorkResult.create when exports.length !== item.exportCount.
In `@packages/jam/jam-host-calls/externalities/refine-externalities.test.ts`:
- Around line 155-161: The test helper's exportSegment currently pushes the
segment into exportSegments regardless of the Result in exportSegmentData;
change exportSegment (the method) to inspect the Result returned from
exportSegmentData.get(segment) and only push to exportSegments when the Result
indicates a successful export (i.e., the Ok/Success variant), leaving error
results untracked; use the existing exportSegmentData and exportSegments fields
to perform this conditional push so failure-path assertions match real refine
externalities behavior.
---
Nitpick comments:
In `@packages/jam/in-core/in-core.ts`:
- Around line 36-44: RefineResult.exports is currently mutable while
RefineItemResult.exports is readonly; update the RefineResult type so its
exports use the same readonly semantics by changing the exports generic from
PerWorkItem<Segment[]> to PerWorkItem<readonly Segment[]> (or an equivalent
readonly Segment[] type) so both RefineResult.exports and
RefineItemResult.exports are consistent; adjust any consuming code or type
assertions that assume mutability of RefineResult.exports (look for usages of
RefineResult and PerWorkItem) and run type checks to ensure no breaks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e2ef29da-ceab-4864-bd4e-ab8e5407a33b
📒 Files selected for processing (6)
packages/jam/in-core/externalities/refine.test.tspackages/jam/in-core/externalities/refine.tspackages/jam/in-core/in-core.tspackages/jam/jam-host-calls/externalities/refine-externalities.test.tspackages/jam/jam-host-calls/externalities/refine-externalities.tspackages/jam/jam-host-calls/refine/export.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jam/jam-host-calls/externalities/refine-externalities.ts
View all
Benchmarks summary: 83/83 OK ✅ |
tomusdrw
approved these changes
Mar 18, 2026
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
2 participants
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.
Export Segment