Block gas cost #708
Block gas cost #708
Conversation
|
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 a new PVM gas-cost test type and runner, introduces a helper to parse/validate test inputs, integrates the runner into the W3F test harness, and implements Interpreter.calculateBlockGasCost to compute per-basic-block gas totals used by the new tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor CLI as Test Runner CLI
participant R as w3f/runners.ts
participant T as PvmGasCostTest
participant H as runPvmGasCostTest
participant I as Interpreter
CLI->>R: Load "gas-cost-tests"
R->>T: fromJson(testContent)
T-->>R: PvmGasCostTest
R->>H: runPvmGasCostTest(testContent)
H->>I: new Interpreter()
H->>I: reset(program, 0, gas=tryAsGas(1000))
H->>I: calculateBlockGasCost()
I-->>H: blockGasCosts (Record<string, number>)
H->>H: assert.deepStrictEqual(result, expected)
H-->>R: test pass/fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Comment |
![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
🧹 Nitpick comments (2)
bin/test-runner/w3f/pvm-gas-cost.ts (2)
18-32: Consider using a private constructor with a static factory method.The class uses definite assignment assertions (
!) for its properties. While this works, consider following the pattern of using a private constructor with a static factory method (likecreate()) for better type safety and initialization guarantees, especially since this class has astatic fromJsonfield similar to classes withstatic Codecfields mentioned in the coding guidelines.Apply this diff to improve type safety:
export class PvmGasCostTest { static fromJson: FromJson<PvmGasCostTest> = { program: fromJson.uint8Array, block_gas_costs: json.fromAny((v) => { if (isBlockCostObject(v)) { return v; } throw new Error(`Expected an object, got ${typeof v} instead.`); }), }; - program!: Uint8Array; - block_gas_costs!: Record<string, number>; + private constructor( + public readonly program: Uint8Array, + public readonly block_gas_costs: Record<string, number>, + ) {} + + static create(program: Uint8Array, block_gas_costs: Record<string, number>): PvmGasCostTest { + return new PvmGasCostTest(program, block_gas_costs); + } }Note: This would require updating the
fromJsonparser to use the factory method.
34-41: Hardcoded gas value may be insufficient for complex programs.The test runner initializes the interpreter with a fixed gas value of 1000. This might be insufficient for programs with many instructions or high gas costs. Consider whether this value should be:
- Configurable via test input
- Calculated based on the program size
- Set to a sufficiently large value to ensure all blocks can be analyzed
Since
calculateBlockGasCost()only analyzes the program structure without executing it, consider using a sufficiently large gas value or making it configurable:export async function runPvmGasCostTest(testContent: PvmGasCostTest) { const pvm = new Interpreter(); - pvm.reset(testContent.program, 0, tryAsGas(1000)); + // Use a large gas value since we're only analyzing structure, not executing + pvm.reset(testContent.program, 0, tryAsGas(1_000_000)); const blockGasCosts = pvm.calculateBlockGasCost(); assert.deepStrictEqual(blockGasCosts, testContent.block_gas_costs); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bin/test-runner/w3f/pvm-gas-cost.ts(1 hunks)bin/test-runner/w3f/runners.ts(2 hunks)packages/core/pvm-interpreter/interpreter.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
packages/core/**/*.ts
⚙️ CodeRabbit configuration file
packages/core/**/*.ts: Core packages must not import any JAM-related packages
(i.e. packages defined underpackages/jam/**)
Files:
packages/core/pvm-interpreter/interpreter.ts
**/*.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:
packages/core/pvm-interpreter/interpreter.tsbin/test-runner/w3f/runners.tsbin/test-runner/w3f/pvm-gas-cost.ts
🧬 Code graph analysis (2)
bin/test-runner/w3f/runners.ts (2)
bin/test-runner/common.ts (1)
runner(15-21)bin/test-runner/w3f/pvm-gas-cost.ts (2)
PvmGasCostTest(18-32)runPvmGasCostTest(34-41)
bin/test-runner/w3f/pvm-gas-cost.ts (2)
packages/core/pvm-interpreter/interpreter.ts (1)
Interpreter(60-352)packages/core/pvm-interpreter/gas.ts (1)
tryAsGas(18-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: run (22.x)
- GitHub Check: run (22.x)
- GitHub Check: state_transition (22.x)
- GitHub Check: test (22.x)
- GitHub Check: benchmarks (22.x)
- GitHub Check: e2e (22.x)
🔇 Additional comments (3)
bin/test-runner/w3f/pvm-gas-cost.ts (1)
6-17: LGTM! Helper functions are well-implemented.The
isPlainObjectandisBlockCostObjectvalidators correctly ensure type safety for the test input structure.bin/test-runner/w3f/runners.ts (1)
42-42: LGTM! Test runner registration follows existing patterns.The new gas-cost-tests runner is properly integrated into the test harness, following the same pattern as other test runners.
Also applies to: 64-64
packages/core/pvm-interpreter/interpreter.ts (1)
328-351: Verify initial block "0" cost in test data
Confirm that the test JSON loaded bybin/test-runner/w3f/pvm-gas-cost.tsincludes (or omits) an entry for block"0"with a zero cost, ensuringcalculateBlockGasCostaligns with expected behavior.
View all
Benchmarks summary: 63/63 OK ✅ |
I added logic to load pvm gas cost tests and simple method to calculate gas cost of all basic blocks