Conversation
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds per-file try-catch to block loading, adjusts state-transition tests to detect and assert rejected blocks based on unchanged state roots, and updates WorkExecResultKind enum with version-gated variants and shifted discriminants. Also changes StateEntries test-comparison method to return a plain object from entries. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as Test Runner
participant BL as Block Loader
participant ST as StateTransition
rect rgba(230,245,255,0.6)
note over BL: Load blocks
TR->>BL: loadBlocks(files)
loop for each file
BL->>BL: try parse
alt genesis.json
BL-->>TR: StateTransitionGenesis.fromJson(...)
else non-genesis
BL-->>TR: StateTransition.fromJson(...)
else parse error
BL-->>TR: skip (ignored)
end
end
end
rect rgba(235,255,235,0.6)
note over TR,ST: Execute state transition test
TR->>ST: runStateTransition(pre, tx)
ST-->>TR: result, post
alt pre.state_root == post.state_root
note over TR: shouldBlockBeRejected=true
TR->>TR: assert !result.ok<br/>assert pre==post
TR-->>TR: return
else result.ok
TR->>TR: assert success path
else error path
TR->>TR: assert error handling
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![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: 1
🧹 Nitpick comments (3)
packages/jam/state-merkleization/state-entries.ts (1)
69-71: Avoid Object.fromEntries here: risk of proto-pollution and key-collision; return a Map of stringified keys instead.Object.fromEntries() on non-string keys coerces them via toString and allows "proto" to mutate the prototype. For test comparisons, deepEqual already special-cases Map, so returning a Map<string, BytesBlob> is both safer and stable.
Apply this diff:
/** When comparing, we can safely ignore `trieCache` and just use entries. */ [TEST_COMPARE_USING]() { - return Object.fromEntries(this.entries); + // Use Map to avoid proto-pollution and preserve key semantics. + const mapped = new Map<string, BytesBlob>(); + for (const [k, v] of this.entries) { + mapped.set(String(k), v); + } + return mapped; }Alternative (if you must keep a plain object):
const out: Record<string, BytesBlob> = Object.create(null); for (const [k, v] of this.entries) out[String(k)] = v; return out;bin/test-runner/state-transition/state-transition.ts (1)
134-141: Good early return on rejection; minor nit: assert on isError for clarity.The block is properly asserted as rejected and state/root invariants are rechecked. Consider asserting stfResult.isError to be explicit.
Optional tweak:
- assert.strictEqual(stfResult.isOk, false, "The block should be rejected, yet we imported it."); + assert.strictEqual(stfResult.isError, true, "The block should be rejected, yet we imported it.");packages/jam/block/work-result.ts (1)
17-29: Consider using a MAX_KIND guard to future-proof the decoder bounds check.Since codeOversize shifts with versions, a single source of truth for the maximum discriminant can improve readability and reduce mistakes if more variants arrive.
Example (no diff needed, just illustrating the pattern):
const MAX_KIND = Compatibility.selectIfGreaterOrEqual({ fallback: WorkExecResultKind.codeOversize, // <0.6.7 -> 4 versions: { [GpVersion.V0_6_7]: WorkExecResultKind.codeOversize }, // >=0.6.7 -> 6 }); // In decoder: if (kind > MAX_KIND) throw new Error(`Invalid WorkExecResultKind: ${kind}`);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (3)
bin/test-runner/state-transition/state-transition.ts(2 hunks)packages/jam/block/work-result.ts(2 hunks)packages/jam/state-merkleization/state-entries.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.ts
⚙️ CodeRabbit configuration file
**/*.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:
bin/test-runner/state-transition/state-transition.tspackages/jam/state-merkleization/state-entries.tspackages/jam/block/work-result.ts
🧠 Learnings (2)
📚 Learning: 2025-06-10T12:06:32.535Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#419
File: packages/jam/database-lmdb/states.test.ts:158-160
Timestamp: 2025-06-10T12:06:32.535Z
Learning: In test code, tomusdrw prefers pragmatic solutions over strict type safety when listing all entries would be cumbersome. Using `Object.assign({}, state)` for state updates in tests is acceptable even if it compromises type safety, prioritizing test maintainability and readability.
Applied to files:
packages/jam/state-merkleization/state-entries.ts
📚 Learning: 2025-06-10T12:20:17.513Z
Learnt from: tomusdrw
PR: FluffyLabs/typeberry#419
File: packages/jam/state/state-inmemory.ts:141-149
Timestamp: 2025-06-10T12:20:17.513Z
Learning: In the `InMemoryService.copyFrom` function in `packages/jam/state/state-inmemory.ts`, duplicate checking for `(hash, length)` pairs in the lookup history is not necessary because the function operates under the assumption that the input `ServiceEntries` comes from an existing well-formed state, which already maintains the invariant of unique lookup history entries per hash and length combination.
Applied to files:
packages/jam/state-merkleization/state-entries.ts
🧬 Code graph analysis (2)
bin/test-runner/state-transition/state-transition.ts (5)
packages/core/json-parser/parse.ts (1)
parseFromJson(6-140)packages/jam/block/block.ts (1)
Block(70-88)packages/jam/node/common.ts (1)
emptyBlock(111-126)packages/jam/transition/block-verifier.ts (1)
BlockVerifier(19-90)packages/core/utils/test.ts (1)
deepEqual(41-187)
packages/jam/block/work-result.ts (1)
packages/core/utils/compatibility.ts (1)
Compatibility(55-111)
🔇 Additional comments (2)
bin/test-runner/state-transition/state-transition.ts (1)
125-125: LGTM: clear early-reject signal derived from unchanged state roots.Simple and robust heuristic; fits the conformance intent.
packages/jam/block/work-result.ts (1)
17-29: Drop import-order concern:Compatibility.overrideis only invoked in tests, so in production the enum discriminants useDEFAULT_VERSION(fromprocess.env) at module load—import order won’t affect them.Likely an incorrect or invalid review comment.
View all
Benchmarks summary: 63/63 OK ✅ |
This was referenced Dec 25, 2025
Merged
3 tasks
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
3 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.
And some extra fixes for running conformance failed cases.