Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis pull request refactors and updates the JAM package across multiple files. It extends JSON parsing for WorkExecResult to recognize a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (13)
💤 Files with no reviewable changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.ts⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (3)packages/jam/transition/externalities/fetch-externalities.ts (2)
packages/jam/transition/statistics.test.ts (4)
packages/jam/transition/reports/input.ts (4)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (35)
Comment |
mateuszsikora
force-pushed
the
ms-update-links-in-reports
branch
from
81d9bed to
9f959bb
Compare
mateuszsikora
force-pushed
the
ms-update-links-in-reports
branch
from
9f959bb to
b898011
Compare
mateuszsikora
requested review from
DrEverr,
skoszuta and
tomusdrw
and removed request for
skoszuta and
tomusdrw
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/jam/transition/reports/verify-basic.ts (1)
6-11: Update the graypaper link version in the constant documentation.The comment block for
MAX_WORK_REPORT_SIZE_BYTESstill references version 0.6.2, but this PR updates all links to version 0.7.2.As per coding guidelines, when making changes to code with comments containing links to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
🔎 Proposed fix
/** * `W_R = 48 * 2**10`: The maximum total size of all output blobs in a work-report, in octets. * - * https://graypaper.fluffylabs.dev/#/5f542d7/41a60041aa00?v=0.6.2 + * https://graypaper.fluffylabs.dev/#/ab2cdbd/41a60041aa00?v=0.7.2 */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/jam/block-json/work-result.tspackages/jam/block/gp-constants.tspackages/jam/block/guarantees.tspackages/jam/transition/externalities/fetch-externalities.tspackages/jam/transition/reports/guarantor-assignment.tspackages/jam/transition/reports/input.tspackages/jam/transition/reports/reports.tspackages/jam/transition/reports/verify-basic.tspackages/jam/transition/reports/verify-contextual.tspackages/jam/transition/reports/verify-credentials.tspackages/jam/transition/reports/verify-order.tspackages/jam/transition/reports/verify-post-signature.tspackages/jam/transition/statistics.test.ts
💤 Files with no reviewable changes (1)
- packages/jam/block/gp-constants.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.ts: rules from./CODESTYLE.mdshould be adhered to.
**/*.ts: Any function whose documentation mention it must not be used in production code,
can be safely used in*.test.tsfiles. Other usage should be carefuly reviewed
and the comment must explain why it's safe to use there.
**/*.ts:asconversions must not be used. Suggest usingtryAsconversion methods.
**/*.ts: Classes withstatic Codecfield must have private constructor and staticcreatemethod.
**/*.ts: Casting abigint(orU64) usingNumber(x)must have an explanation comment why
it is safe.
**/*.ts: When making changes to code with comments containing links (in classes, constants, methods, etc.)
to graypaper.fluffylabs.dev, ensure those links point to the current version for this update.
Files:
packages/jam/transition/reports/verify-order.tspackages/jam/transition/externalities/fetch-externalities.tspackages/jam/transition/statistics.test.tspackages/jam/transition/reports/verify-credentials.tspackages/jam/transition/reports/guarantor-assignment.tspackages/jam/transition/reports/verify-contextual.tspackages/jam/transition/reports/verify-post-signature.tspackages/jam/block/guarantees.tspackages/jam/transition/reports/verify-basic.tspackages/jam/block-json/work-result.tspackages/jam/transition/reports/reports.tspackages/jam/transition/reports/input.ts
🧬 Code graph analysis (3)
packages/jam/transition/externalities/fetch-externalities.ts (2)
packages/core/numbers/index.ts (1)
tryAsU32(45-48)packages/jam/transition/reports/verify-basic.ts (1)
MAX_WORK_REPORT_SIZE_BYTES(11-11)
packages/jam/transition/statistics.test.ts (4)
packages/core/numbers/index.ts (1)
isU32(51-51)packages/jam/transition/reports/verify-basic.ts (1)
MAX_WORK_REPORT_SIZE_BYTES(11-11)packages/jam/block/gp-constants.ts (2)
I(21-21)W_M(45-45)packages/jam/config/chain-spec.ts (1)
EC_SEGMENT_SIZE(36-36)
packages/jam/transition/reports/input.ts (4)
packages/jam/block/guarantees.ts (1)
GuaranteesExtrinsicView(104-104)packages/jam/block/common.ts (1)
TimeSlot(16-16)packages/jam/transition/recent-history.ts (1)
RecentHistoryStateUpdate(36-36)packages/jam/transition/assurances.ts (1)
AssurancesStateUpdate(32-32)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: benchmarks (22.x)
- GitHub Check: Setup test vectors cache
- GitHub Check: e2e (22.x)
🔇 Additional comments (35)
packages/jam/block/guarantees.ts (1)
17-17: Documentation links correctly updated to v0.7.2.All graypaper references have been consistently updated to the new version with the v=0.7.2 query parameter. The URLs maintain the same section references and follow the established documentation linking pattern.
Also applies to: 46-46, 73-73, 88-88
packages/jam/transition/reports/guarantor-assignment.ts (1)
1-76: Documentation updates correctly applied; graypaper links and terminology aligned with v0.7.2.All changes are documentation-only: graypaper links have been updated to v0.7.2 (per coding guidelines requirement), and terminology has been updated consistently (G → M). No functional code changes, function signatures, or control flow modifications. The updated links and comments align correctly with the PR objectives.
packages/jam/transition/reports/reports.ts (1)
1-294: Documentation updates correctly applied; graypaper links updated to v0.7.2 and terminology consistent.All changes are documentation-only: graypaper links have been updated to v0.7.2 (per coding guidelines), terminology has been updated consistently (R→M in ReportsOutput, G/G→M/M in assignment comments), and notation has been refined (τ ′ → τ'). No functional code changes, function signatures, or control flow modifications. The code properly uses type conversion utilities (tryAs, asOpaqueType, asKnownSize) without bare
asconversions. All updates align with the PR objectives.packages/jam/transition/reports/verify-post-signature.ts (1)
24-24: LGTM! Documentation updates align with PR objectives.The graypaper.fluffylabs.dev URLs have been correctly updated to v=0.7.2, and the terminology change from "work item" to "work-digest" is consistent with the PR's broader refactoring goals. These comment-only changes have no functional impact.
Also applies to: 35-35, 49-49, 52-52
packages/jam/transition/reports/verify-order.ts (1)
11-11: Graypaper documentation link correctly updated to v0.7.2.The URL has been properly updated with the version parameter matching the current Graypaper release.
packages/jam/transition/reports/verify-basic.ts (2)
21-21: LGTM! Documentation updated correctly.The graypaper link has been correctly updated to version 0.7.2.
39-39: LGTM! Documentation updated correctly.The graypaper link has been correctly updated to version 0.7.2.
packages/jam/transition/externalities/fetch-externalities.ts (3)
2-2: LGTM! Import consolidation.The imports from gp-constants have been properly consolidated into a single line.
18-18: LGTM! Import added for new constant source.The import for
MAX_WORK_REPORT_SIZE_BYTESfrom verify-basic is correctly added to replace the removed W_R constant.
155-155: LGTM! Correct replacement of W_R constant.The W_R field now uses
MAX_WORK_REPORT_SIZE_BYTESfrom verify-basic.ts, which defines the same value (48 * 2**10). The use oftryAsU32follows the coding guidelines.packages/jam/transition/statistics.test.ts (3)
14-14: LGTM! Import updated correctly.The W_R constant has been removed from the gp-constants import, consistent with the PR objective.
44-44: LGTM! New constant imported.The
MAX_WORK_REPORT_SIZE_BYTESconstant is correctly imported from verify-basic to replace W_R usage in tests.
63-67: LGTM! Test formulas updated correctly.The test formulas now use
MAX_WORK_REPORT_SIZE_BYTESinstead of W_R, maintaining the same semantic meaning while using the new constant source.packages/jam/transition/reports/verify-credentials.ts (4)
31-33: LGTM! Terminology and link updated correctly.The comment correctly updates the terminology from "reporters set R" to "reporters set G" and the graypaper link to version 0.7.2, aligning with the PR objectives.
47-47: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2 with the new commit hash.
92-93: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
120-120: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
packages/jam/transition/reports/verify-contextual.ts (9)
21-21: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
54-58: LGTM! Documentation improved with proper formatting.The comment has been converted to a multi-line JSDoc format and the graypaper link updated to version 0.7.2.
73-73: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
138-138: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
155-157: LGTM! Documentation improved.The comment formatting has been improved and the graypaper link updated to version 0.7.2.
189-189: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
204-204: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
246-246: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
287-287: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
packages/jam/transition/reports/input.ts (5)
21-26: LGTM! Terminology and documentation updated.The terminology has been correctly updated from "work outputs" to "work-digests" and the graypaper link updated to version 0.7.2, aligning with the PR objectives.
32-35: LGTM! Terminology and documentation updated.The terminology has been correctly updated from "work outputs" to "work-digests" and the graypaper link updated to version 0.7.2.
42-46: LGTM! Documentation improved with proper formatting.The comment has been expanded to a multi-line JSDoc format with a clearer description and the graypaper link updated to version 0.7.2.
50-51: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
57-57: LGTM! Documentation link updated.The graypaper link has been correctly updated to version 0.7.2.
packages/jam/block-json/work-result.ts (4)
18-18: LGTM! New field added to parser.The
output_oversizefield has been correctly added to the JSON parser configuration, following the same pattern as other optional fields.
21-21: LGTM! Destructuring updated.The destructuring correctly includes the new
output_oversizefield.
37-39: LGTM! Logic added for output_oversize handling.The logic correctly maps
output_oversize === nulltoWorkExecResultKind.digestTooBig, following the same pattern as other error cases likecode_oversize. This aligns with the PR's terminology updates around work outputs/digests.
51-51: LGTM! Type definition updated.The
JsonWorkExecResulttype correctly includes the newoutput_oversize?: nullfield.
View all
Benchmarks summary: 83/83 OK ✅ |
2 participants
Changes:
G=>M, work outout => work digest)W_Rfrom GP consts (we had this const in two places)