Conversation
Contributor
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughConsolidates work-package inputs into a new WorkPackageFetchData and threads it through authorization and refinement. IsAuthorized.invoke and Refine.invoke signatures changed to accept WorkPackageFetchData; buildWorkPackageFetchData was added to precompute package view and work-item summaries. New in-core externalities implementations (IsAuthorizedFetchExternalities, RefineFetchExternalities) and companion helpers (encodeWorkItemSummary, u64ToArrayIndex) were added. Tests updated/added to exercise the new fetch-data-driven behavior. Transition externalities barrel removed legacy re-exports. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🤖 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/is-authorized.ts`:
- Around line 46-52: The invoke function reads
authCodeHost/authCodeHash/authConfiguration from workPackage while other code
reads authToken/authConfiguration from packageFetchData, risking divergent
inputs; update invoke in is-authorized.ts to use a single source of truth by
deriving all authorization inputs from packageFetchData (or alternatively
require workPackage as canonical) and remove mixed reads, and add a strict
fail-fast check that compares authCodeHost/authCodeHash/authConfiguration from
workPackage against the corresponding values in packageFetchData (and/or
authToken/authorizerHash) before running invoke/PVM so any mismatch throws an
error; reference the invoke method, workPackage, packageFetchData, authCodeHost,
authCodeHash, authConfiguration, authToken and authorizerHash when making the
change.
In `@packages/jam/transition/externalities/refine-fetch-externalities.test.ts`:
- Around line 179-183: The test currently only asserts the invalid work-item
selection path; add an assertion that covers the invalid segment-index branch by
invoking workItemImport with a valid work-item index (e.g., tryAsU64(0)) and an
out-of-range import index (e.g., tryAsU64(10)) and assert it returns null.
Locate the test using prepareRefineData and workItemImport and add the new
assert.strictEqual(ext.workItemImport(tryAsU64(0), tryAsU64(10)), null) to the
same it block.
- Around line 168-170: The test fixture for ImportedSegment is using unsafe
casts ("as never") which bypass the module's opaque/index typing; replace the
casts by constructing the real typed values instead: use the project's
opaque/index conversion or validation helper (e.g., the
tryAs*/ensure*/fromNumber helper for the index) to produce a properly typed
index for ImportedSegment, and pass Bytes.fromBlob(segBytes, 16) without erasing
types so the ImportedSegment.data matches its declared Bytes type; remove both
"as never" occurrences and build the object via the real helpers referenced by
ImportedSegment, asKnownSize, Bytes.fromBlob and segBytes.
In `@packages/jam/transition/externalities/refine-fetch-externalities.ts`:
- Around line 8-22: The import of ImportedSegment and PerWorkItem from
in-core/refine.ts creates a circular dependency; extract the shared types into a
neutral module (e.g., a new refine-types module) and export ImportedSegment and
PerWorkItem (and any other types both files need) from there, then update
refine-fetch-externalities.ts to import ImportedSegment and PerWorkItem from
that new neutral module and likewise update in-core/refine.ts to import those
types from the same module so both sides no longer import each other.
🪄 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: 2c684f58-6b9b-4375-a3fe-55c5c3fbe1fc
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
packages/jam/in-core/in-core.tspackages/jam/in-core/is-authorized.test.tspackages/jam/in-core/is-authorized.tspackages/jam/in-core/refine.tspackages/jam/transition/externalities/fetch-externalities.tspackages/jam/transition/externalities/is-authorized-fetch-externalities.test.tspackages/jam/transition/externalities/is-authorized-fetch-externalities.tspackages/jam/transition/externalities/refine-fetch-externalities.test.tspackages/jam/transition/externalities/refine-fetch-externalities.ts
# Conflicts: # packages/jam/in-core/refine.ts # packages/jam/transition/externalities/refine-fetch-externalities.test.ts # packages/jam/transition/externalities/refine-fetch-externalities.ts
Member
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/jam/in-core/externalities/is-authorized-fetch.ts (1)
14-17: Prefer a static builder over a public constructor here.This is part of the refactor’s new externality surface, so exposing
new IsAuthorizedFetchExternalities(...)makes call sites depend on construction details. Acreate()/new()factory with a private constructor keeps this aligned with the repo pattern and leaves room for validation later.Suggested shape
export class IsAuthorizedFetchExternalities implements general.IIsAuthorizedFetch { readonly context = general.FetchContext.IsAuthorized; - constructor( + static create(chainSpec: ChainSpec, pkg: WorkPackageFetchData) { + return new IsAuthorizedFetchExternalities(chainSpec, pkg); + } + + private constructor( private readonly chainSpec: ChainSpec, private readonly pkg: WorkPackageFetchData, ) {}As per coding guidelines, "keep constructors private and push logic into static builder methods (avoid constructor overloading)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/externalities/is-authorized-fetch.ts` around lines 14 - 17, Make the public constructor of IsAuthorizedFetchExternalities private and add a static factory method (e.g., create(chainSpec: ChainSpec, pkg: WorkPackageFetchData): IsAuthorizedFetchExternalities) that constructs and returns the instance; update call sites to use IsAuthorizedFetchExternalities.create(...) so callers don't depend on construction details and you can add validation later. Ensure the class name IsAuthorizedFetchExternalities and parameter types ChainSpec and WorkPackageFetchData are used exactly as in the diff.packages/jam/in-core/externalities/refine-fetch.ts (2)
24-29: Prefer an opaque sized integer overnumberforcurrentWorkItemIndex.Using plain
numberhere weakens range guarantees; aU32(or equivalent opaque bounded type) plus checked conversion would better match surrounding index-safety patterns.As per coding guidelines, “Prefer Opaque sized-integer types (U16/U32/U64) and use tryAs* / ensure+check for conversions rather than unsafe casts.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/externalities/refine-fetch.ts` around lines 24 - 29, RefineFetchData currently types currentWorkItemIndex as number; change it to the opaque sized-integer type used elsewhere (e.g. U32) and import that type, then update all places that construct RefineFetchData to convert/validate with the project’s checked conversion helpers (e.g. tryAsU32 or ensure+check) instead of casting, and adjust any downstream consumers to accept U32 (or call .toNumber() only where a plain number is explicitly required). Ensure the symbol RefineFetchData and field currentWorkItemIndex are updated, and that conversion calls reference the existing checked conversion utilities used across WorkPackageFetchData handling.
40-43: Use a private constructor with a static builder for this refactor.Please align this class construction pattern with the repo rule for this refactor.
As per coding guidelines, “keep constructors private and push logic into static builder methods (avoid constructor overloading).”Suggested shape
export class RefineFetchExternalities implements general.IRefineFetch { @@ - constructor( + private constructor( private readonly chainSpec: ChainSpec, private readonly data: RefineFetchData, ) {} + + static create(chainSpec: ChainSpec, data: RefineFetchData): RefineFetchExternalities { + return new RefineFetchExternalities(chainSpec, data); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/externalities/refine-fetch.ts` around lines 40 - 43, Make the constructor of the RefineFetch class private and add a static builder method (e.g., static create(chainSpec: ChainSpec, data: RefineFetchData): RefineFetch) that calls the private constructor; move any construction logic out of the constructor into that static method and update all call sites to use RefineFetch.create(...) instead of new RefineFetch(...). Ensure the new static method preserves types and visibility, and keep the original constructor signature but change its modifier to private so instantiation only happens via the builder.
🤖 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-fetch.ts`:
- Around line 62-89: Both workItemExtrinsic and workItemImport use
this.data.currentWorkItemIndex directly when workItem === null, which can index
out-of-range and cause a crash; change the ternary so that when workItem ===
null you convert and guard the currentWorkItemIndex via
u64ToArrayIndex(this.data.currentWorkItemIndex, this.data.extrinsics.length) in
workItemExtrinsic and u64ToArrayIndex(this.data.currentWorkItemIndex,
this.data.imports.length) in workItemImport, then return null if that conversion
yields null before accessing perItem or perItem.length.
In `@packages/jam/in-core/refine.ts`:
- Around line 72-80: The invoke() implementation mixes two sources for the
“current” work item (the explicit parameter item versus the entry in
packageFetchData/currentWorkItemIndex), which can cause the VM to operate on one
item while fetch-based summaries/payloads come from another; fix by deriving the
current item consistently from packageFetchData using idx (e.g. let currentItem
= packageFetchData.items[idx]) when preparing code lookup, gas, payload hash,
PVM args and when wiring fetch host calls, or if you prefer a minimal change add
a strict fail-fast check in invoke() (and the similar logic around lines
119–126) that compares the passed-in item to packageFetchData[idx] and
immediately return/error if they differ before creating externalities. Ensure
all uses (code lookup, gas, payload hash, PVM args, fetch wiring) reference the
single canonical currentItem.
---
Nitpick comments:
In `@packages/jam/in-core/externalities/is-authorized-fetch.ts`:
- Around line 14-17: Make the public constructor of
IsAuthorizedFetchExternalities private and add a static factory method (e.g.,
create(chainSpec: ChainSpec, pkg: WorkPackageFetchData):
IsAuthorizedFetchExternalities) that constructs and returns the instance; update
call sites to use IsAuthorizedFetchExternalities.create(...) so callers don't
depend on construction details and you can add validation later. Ensure the
class name IsAuthorizedFetchExternalities and parameter types ChainSpec and
WorkPackageFetchData are used exactly as in the diff.
In `@packages/jam/in-core/externalities/refine-fetch.ts`:
- Around line 24-29: RefineFetchData currently types currentWorkItemIndex as
number; change it to the opaque sized-integer type used elsewhere (e.g. U32) and
import that type, then update all places that construct RefineFetchData to
convert/validate with the project’s checked conversion helpers (e.g. tryAsU32 or
ensure+check) instead of casting, and adjust any downstream consumers to accept
U32 (or call .toNumber() only where a plain number is explicitly required).
Ensure the symbol RefineFetchData and field currentWorkItemIndex are updated,
and that conversion calls reference the existing checked conversion utilities
used across WorkPackageFetchData handling.
- Around line 40-43: Make the constructor of the RefineFetch class private and
add a static builder method (e.g., static create(chainSpec: ChainSpec, data:
RefineFetchData): RefineFetch) that calls the private constructor; move any
construction logic out of the constructor into that static method and update all
call sites to use RefineFetch.create(...) instead of new RefineFetch(...).
Ensure the new static method preserves types and visibility, and keep the
original constructor signature but change its modifier to private so
instantiation only happens via the builder.
🪄 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: 789b610f-80e0-498e-aa8e-ad6b03b58434
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (14)
packages/jam/in-core/externalities/index.tspackages/jam/in-core/externalities/is-authorized-fetch.test.tspackages/jam/in-core/externalities/is-authorized-fetch.tspackages/jam/in-core/externalities/refine-fetch.test.tspackages/jam/in-core/externalities/refine-fetch.tspackages/jam/in-core/in-core.tspackages/jam/in-core/is-authorized.test.tspackages/jam/in-core/is-authorized.tspackages/jam/in-core/refine.tspackages/jam/transition/externalities/fetch-externalities.tspackages/jam/transition/externalities/index.tspackages/jam/transition/externalities/is-authorized-fetch-externalities.tspackages/jam/transition/externalities/refine-fetch-externalities.test.tspackages/jam/transition/externalities/refine-fetch-externalities.ts
💤 Files with no reviewable changes (4)
- packages/jam/transition/externalities/index.ts
- packages/jam/transition/externalities/refine-fetch-externalities.test.ts
- packages/jam/transition/externalities/is-authorized-fetch-externalities.ts
- packages/jam/transition/externalities/refine-fetch-externalities.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.
Related: #694
Closes: #880