Open
Conversation
Contributor
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR splits authorization and refinement into distinct components: adds IsAuthorized and Refine modules that run PVM invocations for package authorization and per-item refinement. WorkPackage fields Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 5
🧹 Nitpick comments (3)
packages/jam/in-core/refine.test.ts (1)
1-7: Placeholder test file noted.The TODO comment clearly documents the intent to add
Refine.invoke()tests following theis-authorized.test.tspattern. Consider tracking this with an issue to ensure follow-up.Would you like me to open an issue to track adding the refine-specific PVM invocation tests?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/refine.test.ts` around lines 1 - 7, Create a tracked issue to add refine-specific PVM invocation tests for Refine.invoke(), referencing the existing is-authorized.test.ts pattern; the issue should state that tests should call Refine.invoke() directly, specify expected behaviors and edge cases to cover, and link to this placeholder test so someone can pick it up later.packages/jam/in-core/is-authorized.test.ts (1)
28-48: Consider extractingcreateServiceto a shared test utility.This helper is nearly identical to the one in
in-core.test.ts(lines 35-55). Extracting to a shared test utility would reduce duplication and ensure consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/is-authorized.test.ts` around lines 28 - 48, The duplicate createService helper should be extracted to a shared test utility: move the createService implementation (which constructs an InMemoryService using ServiceAccountInfo.create, PreimageItem.create, HashDictionary.fromEntries, etc.) into a new exported test helper module, export any dependent types or helper functions (e.g., OpaqueHash/BytesBlob typings if needed), and replace the local copies in both in-core.test.ts and is-authorized.test.ts with imports from that shared module; ensure the new module exports the same function signature so tests using createService (and references to InMemoryService, codeHash, code) need only import it and all existing calls continue to work.packages/jam/transition/externalities/is-authorized-fetch-externalities.ts (1)
7-53: Implementation correctly provides auth fields; stub methods are safe placeholders.The class properly implements the essential auth-related methods (
authToken(),authConfiguration(),constants()). The stub methods returningBytesBlob.empty()ornullare safe per thegetValue()implementation infetch.ts(Context snippet 1), which handles these return values without panic.The TODOs clearly document future work needed for full fetch support during authorization.
Would you like me to open issues to track implementing the stubbed methods (
workPackage,refineContext,allWorkItems,oneWorkItem,workItemPayload)?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/transition/externalities/is-authorized-fetch-externalities.ts` around lines 7 - 53, Summary: IsAuthorizedFetchExternalities implements auth fields but leaves several methods as safe stubs. Fix: keep current behavior but create tracking issues (or TODOs in your tracker) to implement the stubbed methods: workPackage(), refineContext(), allWorkItems(), oneWorkItem(U64), and workItemPayload(U64) on the IsAuthorizedFetchExternalities class so they return the correct encoded values (E(p) for workPackage, encoded refinement context, encoded work items list, single work item summary, and work item payload respectively); include acceptance criteria in each issue that references these method names and requires returning non-empty BytesBlob or a valid BytesBlob|null consistent with fetch.getValue() expectations and add unit tests covering each implemented method.
🤖 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 44-51: The invoke function currently only accepts
authToken/authConfiguration and forwards those into
IsAuthorizedFetchExternalities, dropping the encoded work package, refine
context, and work-item summaries/payloads; update the IsAuthorized.invoke
signature to also accept workPackage, context, and items (the encoded work
package, refine context, and work-item list) and pass those through into
IsAuthorizedFetchExternalities so FETCH handlers see the real package contents
(also apply the same change where invoke is declared/used around lines 80-83).
Locate the invoke method and any callers, add parameters for the work package,
context, and items, and ensure IsAuthorizedFetchExternalities is
constructed/called with these values instead of only token/configuration.
In `@packages/jam/in-core/refine.ts`:
- Around line 15-16: The cycle is caused by refine.ts importing types
(ImportedSegment, PerWorkItem, RefineItemResult) from in-core.ts while
in-core.ts imports Refine from refine.ts; break it by extracting those three
type definitions into a new module (e.g., in-core-types.ts or refine-types.ts)
and have both refine.ts and in-core.ts import the types from that new file;
update imports in RefineExternalitiesImpl (refine.ts) and wherever in-core.ts
referenced those types to point to the new module and remove the direct import
of in-core.ts from refine.ts.
- Around line 53-63: The invoke() path currently drops per-item data when
constructing RefineFetchExternalities so item-dependent host calls
(payload/imports/extrinsics/work-item fetches) have no backing data; update
invoke() and createRefineExternalities() to accept and pass the current item’s
payload, imports (allImports[idx]), and extrinsics (allExtrinsics[idx]) into the
RefineFetchExternalities constructor, and modify RefineFetchExternalities to
store and use those per-item values when servicing import/extrinsic and
work-item fetch kinds (ensure the work-item payload/summary source is passed
through and used by the fetch handlers).
- Around line 121-135: The branch that handles the incorrect number of exports
returns the actual exports array, which later gets used to advance exportOffset;
change the return so it discards the exports when the counts mismatch (i.e.,
return an empty array instead of the value from
externalities.refine.getExportedSegments()) — update the block that checks
exports.length !== item.exportCount (where exports is from
externalities.refine.getExportedSegments()) to return exports: [] while keeping
the existing WorkResult.create(...) and WorkRefineLoad.create(...) payload
(exportedSegments should remain 0).
In `@packages/jam/transition/externalities/refine-fetch-externalities.ts`:
- Around line 23-26: The unimplemented refine-fetch methods (e.g.,
authorizerTrace) currently return BytesBlob.empty(), which makes the runtime
treat them as successful zero-length fetches; change each unimplemented method
(authorizerTrace, packageTrace, packageKey, packageProof, authorizerKey,
authorizerProof, contextTrace — the methods in refine-fetch-externalities.ts
around the shown snippet) to return null/HostCallResult.NONE instead of
BytesBlob.empty(), and update their return types to allow null (e.g., BytesBlob
| null) so Fetch.execute() sees a genuine "no result" rather than a synthetic
empty blob.
---
Nitpick comments:
In `@packages/jam/in-core/is-authorized.test.ts`:
- Around line 28-48: The duplicate createService helper should be extracted to a
shared test utility: move the createService implementation (which constructs an
InMemoryService using ServiceAccountInfo.create, PreimageItem.create,
HashDictionary.fromEntries, etc.) into a new exported test helper module, export
any dependent types or helper functions (e.g., OpaqueHash/BytesBlob typings if
needed), and replace the local copies in both in-core.test.ts and
is-authorized.test.ts with imports from that shared module; ensure the new
module exports the same function signature so tests using createService (and
references to InMemoryService, codeHash, code) need only import it and all
existing calls continue to work.
In `@packages/jam/in-core/refine.test.ts`:
- Around line 1-7: Create a tracked issue to add refine-specific PVM invocation
tests for Refine.invoke(), referencing the existing is-authorized.test.ts
pattern; the issue should state that tests should call Refine.invoke() directly,
specify expected behaviors and edge cases to cover, and link to this placeholder
test so someone can pick it up later.
In `@packages/jam/transition/externalities/is-authorized-fetch-externalities.ts`:
- Around line 7-53: Summary: IsAuthorizedFetchExternalities implements auth
fields but leaves several methods as safe stubs. Fix: keep current behavior but
create tracking issues (or TODOs in your tracker) to implement the stubbed
methods: workPackage(), refineContext(), allWorkItems(), oneWorkItem(U64), and
workItemPayload(U64) on the IsAuthorizedFetchExternalities class so they return
the correct encoded values (E(p) for workPackage, encoded refinement context,
encoded work items list, single work item summary, and work item payload
respectively); include acceptance criteria in each issue that references these
method names and requires returning non-empty BytesBlob or a valid
BytesBlob|null consistent with fetch.getValue() expectations and add unit tests
covering each implemented method.
🪄 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: c255ebad-cbee-4b1c-be77-b0883dc103af
📒 Files selected for processing (15)
bin/test-runner/w3f/codec/work-package.tspackages/jam/block/work-package.tspackages/jam/executor/pvm-executor.tspackages/jam/in-core/fixtures/authorizer.pvmpackages/jam/in-core/in-core.test.tspackages/jam/in-core/in-core.tspackages/jam/in-core/is-authorized.test.tspackages/jam/in-core/is-authorized.tspackages/jam/in-core/refine.test.tspackages/jam/in-core/refine.tspackages/jam/jam-host-calls/general/fetch.test.tspackages/jam/jam-host-calls/general/fetch.tspackages/jam/transition/externalities/index.tspackages/jam/transition/externalities/is-authorized-fetch-externalities.tspackages/jam/transition/externalities/refine-fetch-externalities.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/jam/in-core/refine.ts (2)
120-127:⚠️ Potential issue | 🔴 CriticalPer-item fetch data is still not wired into refine externalities.
payload,imports, andextrinsicsare collected but not used to back fetch host calls;RefineFetchExternalitiesis still created with onlychainSpec. Item-dependent fetches will not resolve correctly.🐛 Minimal direction to fix
- const externalities = this.createRefineExternalities({ - payload: item.payload, - imports: allImports, - extrinsics: allExtrinsics, + const externalities = this.createRefineExternalities({ + payload: item.payload, + imports, + extrinsics, currentServiceId: item.service, lookupState, exportOffset, }); private createRefineExternalities(args: { payload: BytesBlob; - imports: PerWorkItem<ImportedSegment[]>; - extrinsics: PerWorkItem<WorkItemExtrinsic[]>; + imports: ImportedSegment[]; + extrinsics: WorkItemExtrinsic[]; currentServiceId: ServiceId; lookupState: State; exportOffset: number; }): RefineHostCallExternalities { - const fetchExternalities = new RefineFetchExternalities(this.chainSpec); + const fetchExternalities = new RefineFetchExternalities( + this.chainSpec, + args.payload, + args.imports, + args.extrinsics, + );(plus corresponding constructor/handler support in
packages/jam/transition/externalities/refine-fetch-externalities.ts)Also applies to: 231-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/refine.ts` around lines 120 - 127, The createRefineExternalities call is only passing chainSpec and thus per-item fetch data (payload, imports, extrinsics, currentServiceId, exportOffset, lookupState) never reaches RefineFetchExternalities; update createRefineExternalities invocation (and the RefineFetchExternalities constructor/handler in packages/jam/transition/externalities/refine-fetch-externalities.ts) to accept and store the item-specific fields (payload, imports, extrinsics, currentServiceId, exportOffset, lookupState) so that fetch host calls inside RefineFetchExternalities use the item-scoped data when resolving fetches.
141-145:⚠️ Potential issue | 🟠 MajorDiscard exports when export count validation fails.
When
exports.length !== item.exportCount, returningexportsleaks invalid segments into downstream offset/count aggregation.✅ Minimal fix
if (exports.length !== item.exportCount) { return { - exports, + exports: [], result: WorkResult.create({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/refine.ts` around lines 141 - 145, The current branch returns the captured exports when exports.length !== item.exportCount which leaks invalid segments; change the early-return path in refine (the block that calls externalities.refine.getExportedSegments() and compares exports.length to item.exportCount) to discard the exported segments—return an empty exports array (or omit the exports field) alongside the WorkResult.create(...) instead of returning the collected exports so downstream offset/count aggregation does not receive invalid segments.
🧹 Nitpick comments (2)
packages/jam/in-core/in-core.ts (1)
42-50: Prefer a private constructor + static factory for collaborator wiring.
InCoreis an updated module in this PR, and the constructor now contains composition logic. Please move creation to a static factory (create(...)) and keep constructor private.♻️ Suggested refactor
export class InCore { private readonly isAuthorized: IsAuthorized; private readonly refineItem: Refine; - constructor( + static create( + chainSpec: ChainSpec, + states: StatesDb, + pvmBackend: PvmBackend, + blake2b: Blake2b, + ) { + return new InCore(chainSpec, states, pvmBackend, blake2b); + } + + private constructor( public readonly chainSpec: ChainSpec, private readonly states: StatesDb, pvmBackend: PvmBackend, blake2b: Blake2b, ) {As per coding guidelines, “Prefer Rust-style static builder methods and keep constructors private; 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/in-core.ts` around lines 42 - 50, The InCore constructor currently performs collaborator wiring (instantiating IsAuthorized and Refine); change this by making the class constructor private and moving the wiring into a static async/sync factory method named create(chainSpec, states, pvmBackend, blake2b) which returns a new InCore instance; ensure create constructs IsAuthorized and Refine (using the same parameters) and assigns them to the instance before returning, while the private constructor only accepts already-initialized dependencies (chainSpec, states, isAuthorized, refineItem) to preserve encapsulation and follow the Rust-style static builder guideline.packages/jam/in-core/refine.ts (1)
67-71: Use static factory + private constructor for new module initialization.For this newly added module, prefer static creation and keep constructor private.
As per coding guidelines, “Prefer Rust-style static builder methods and keep constructors private; 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/refine.ts` around lines 67 - 71, Make the constructor private and provide a static factory method to create instances: change the current public constructor(...) to a private constructor(chainSpec, pvmBackend, blake2b) and add a static method (e.g., create or newInstance) that accepts chainSpec, pvmBackend, blake2b, constructs and returns the class instance; update any call sites to use the new static factory instead of new constructor. Ensure symbols referenced are constructor, chainSpec, pvmBackend, blake2b and the new static factory name you choose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/jam/in-core/refine.ts`:
- Around line 120-127: The createRefineExternalities call is only passing
chainSpec and thus per-item fetch data (payload, imports, extrinsics,
currentServiceId, exportOffset, lookupState) never reaches
RefineFetchExternalities; update createRefineExternalities invocation (and the
RefineFetchExternalities constructor/handler in
packages/jam/transition/externalities/refine-fetch-externalities.ts) to accept
and store the item-specific fields (payload, imports, extrinsics,
currentServiceId, exportOffset, lookupState) so that fetch host calls inside
RefineFetchExternalities use the item-scoped data when resolving fetches.
- Around line 141-145: The current branch returns the captured exports when
exports.length !== item.exportCount which leaks invalid segments; change the
early-return path in refine (the block that calls
externalities.refine.getExportedSegments() and compares exports.length to
item.exportCount) to discard the exported segments—return an empty exports array
(or omit the exports field) alongside the WorkResult.create(...) instead of
returning the collected exports so downstream offset/count aggregation does not
receive invalid segments.
---
Nitpick comments:
In `@packages/jam/in-core/in-core.ts`:
- Around line 42-50: The InCore constructor currently performs collaborator
wiring (instantiating IsAuthorized and Refine); change this by making the class
constructor private and moving the wiring into a static async/sync factory
method named create(chainSpec, states, pvmBackend, blake2b) which returns a new
InCore instance; ensure create constructs IsAuthorized and Refine (using the
same parameters) and assigns them to the instance before returning, while the
private constructor only accepts already-initialized dependencies (chainSpec,
states, isAuthorized, refineItem) to preserve encapsulation and follow the
Rust-style static builder guideline.
In `@packages/jam/in-core/refine.ts`:
- Around line 67-71: Make the constructor private and provide a static factory
method to create instances: change the current public constructor(...) to a
private constructor(chainSpec, pvmBackend, blake2b) and add a static method
(e.g., create or newInstance) that accepts chainSpec, pvmBackend, blake2b,
constructs and returns the class instance; update any call sites to use the new
static factory instead of new constructor. Ensure symbols referenced are
constructor, chainSpec, pvmBackend, blake2b and the new static factory name you
choose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4fea1f7d-3cee-4ecd-8164-7c4a0873bb29
📒 Files selected for processing (2)
packages/jam/in-core/in-core.tspackages/jam/in-core/refine.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jam/in-core/refine.ts (1)
173-188: TODO: Output size validation is not implemented.Line 176 notes that output size should be verified and potentially return
digestTooBig. If there's a spec-defined maximum digest size, this check should be added before shipping to prevent oversized results.Would you like me to help identify the max digest size from the Gray Paper and draft the validation logic, or open an issue to track this?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/jam/in-core/refine.ts` around lines 173 - 188, extractWorkResult currently returns WorkExecResult.ok(BytesBlob.blobFrom(slice)) without validating output size; add a size check against the protocol max digest size before calling BytesBlob.blobFrom. Define or import a MAX_DIGEST_SIZE constant (e.g., MAX_DIGEST_SIZE or SPEC_MAX_DIGEST_BYTES), compute slice.length (or slice.byteLength) and if it exceeds the max return WorkExecResult.error(WorkExecResultKind.digestTooBig) instead of ok; otherwise continue to create the blob. Update the TODO comment and add a unit test covering both under-limit and over-limit cases for extractWorkResult.
🤖 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/refine.ts`:
- Around line 239-246: RefineFetchExternalities is being constructed with only
chainSpec but its methods (workItemExtrinsic, workItemImport, workPackage,
allWorkItems) require work-item-specific data; update the
RefineFetchExternalities class to accept and store payload, imports, and
extrinsics (e.g., add constructor params and private fields for payload,
imports, extrinsics and use them in the incomplete methods) and then wire these
into the instantiation in refine.ts by passing the corresponding args (e.g.,
args.payload, args.imports, args.extrinsics) when creating fetchExternalities;
leave RefineExternalitiesImpl.create usage unchanged.
---
Nitpick comments:
In `@packages/jam/in-core/refine.ts`:
- Around line 173-188: extractWorkResult currently returns
WorkExecResult.ok(BytesBlob.blobFrom(slice)) without validating output size; add
a size check against the protocol max digest size before calling
BytesBlob.blobFrom. Define or import a MAX_DIGEST_SIZE constant (e.g.,
MAX_DIGEST_SIZE or SPEC_MAX_DIGEST_BYTES), compute slice.length (or
slice.byteLength) and if it exceeds the max return
WorkExecResult.error(WorkExecResultKind.digestTooBig) instead of ok; otherwise
continue to create the blob. Update the TODO comment and add a unit test
covering both under-limit and over-limit cases for extractWorkResult.
🪄 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: 606f1c0c-2dee-44d0-8f98-200714bd5aa6
📒 Files selected for processing (1)
packages/jam/in-core/refine.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