✅ test(trie,receipt-manager): raise coverage with real-object tests + fix EMPTY_STATE_ROOT proxy invariant - #2090
✅ test(trie,receipt-manager): raise coverage with real-object tests + fix EMPTY_STATE_ROOT proxy invariant#2090roninjin10 wants to merge 2 commits into
Conversation
…nd fix EMPTY_STATE_ROOT proxy invariant <prompt> LANE: Raise test coverage on under-tested core packages. Pick 2-4 under-tested packages, write meaningful tests (real edge cases, error paths, boundary conditions), never mock when avoidable, run suites and report coverage deltas. </prompt> @tevm/trie (lines 46.15% -> 100%, branches 50% -> 100%, functions 30.76% -> 100%): - New EMPTY_STATE_ROOT.spec.ts: pins the well-known empty root hash, copy semantics of .buffer/.subarray, immutability of every mutating Uint8Array method, proxy traps for index vs non-index properties (including the non-canonical '01' property edge case), detached method binding, and a deterministic non-empty genesisStateRoot. - Fix: the proxy get trap bound every function value, but the mutator overrides installed via Object.defineProperties are non-configurable data properties, so even reading .subarray/.fill threw a proxy-invariant TypeError before the intended implementations could run. The trap now returns non-configurable own data properties as-is, making the documented immutable-error and copy semantics actually reachable. @tevm/receipt-manager (lines 50.4% -> 100%, statements 46.71% -> 98.54%, branches 42.68% -> 95.12%, functions 52% -> 100%): - New ReceiptsManager.real.spec.ts uses a real createChain blockchain, real signed transactions, and real createBlock blocks (no mocks): receipt save/get roundtrip, bloom recomputation verified against an independently constructed Bloom, txType attachment, getReceiptByTxHash with cumulative log index, delete semantics including the reorg case where a tx index is owned by a different canonical block, getLogs address/topic filters (positional, null wildcard, OR arrays), block-range and count limits, and defensive error paths.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds real-chain integration tests for ChangesReceipt manager integration coverage
Immutable empty state root
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/receipt-manager/src/ReceiptsManager.real.spec.ts (2)
262-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the block-number/hash assertion for
logs[1].The ternary-based expression on Lines 271-273 conflates a block-number check with a hash comparison, producing confusing failure output if it ever fails.
Proposed fix
- expect(bytesToHex(logs[1]?.block.header.number === 2n ? logs[1].block.hash() : new Uint8Array())).toBe( - bytesToHex((await chain.getBlock(2n)).hash()), - ) + expect(logs[1]?.block.header.number).toBe(2n) + expect(bytesToHex(logs[1]!.block.hash())).toBe(bytesToHex((await chain.getBlock(2n)).hash()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/receipt-manager/src/ReceiptsManager.real.spec.ts` around lines 262 - 274, In the test case returning logs across a block range, update the logs[1] assertion to verify its block number separately from its block hash. Remove the ternary expression and compare logs[1].block.header.number directly to 2n, then compare logs[1].block.hash() with the expected block hash so failures identify the specific mismatch.
250-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an explicit return type to
setupTwoBlocks.Unlike the other local helpers in this file (
makeLog,makeReceipt,makeBlock, etc.),setupTwoBlockshas no explicit return type.Proposed fix
- const setupTwoBlocks = async () => { + const setupTwoBlocks = async (): Promise<{ block1: Block; block2: Block; tx1: TypedTransaction; tx2: TypedTransaction }> => {As per coding guidelines,
**/*.{ts,tsx}: "We always explicitly type return types."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/receipt-manager/src/ReceiptsManager.real.spec.ts` around lines 250 - 260, Update the local setupTwoBlocks helper to declare an explicit return type describing its returned block and transaction objects, while preserving its existing setup behavior and returned properties.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/receipt-manager/src/ReceiptsManager.real.spec.ts`:
- Line 11: Update the test setup around the top-level common and the
block/transaction builders so each test uses one isolated Common instance
consistently. Ensure the builders no longer close over the shared common or
common.ethjsCommon; create or reuse the per-test optimism.copy() instance for
chain construction and builder calls.
In `@packages/trie/src/EMPTY_STATE_ROOT.js`:
- Around line 77-80: Add complete JSDoc immediately above the proxy’s get trap,
documenting its parameters, return value, and possible TypeError via `@param`,
`@returns`, and `@throws` tags. Include a working `@example` with the necessary
imports, while retaining the existing inline explanation of non-configurable
property handling.
- Around line 81-84: Update the descriptor fast path in the proxy get logic to
return descriptor.value only when the own data property is both non-configurable
and non-writable, adding the descriptor.writable check while preserving existing
behavior for other properties.
In `@packages/trie/src/EMPTY_STATE_ROOT.spec.ts`:
- Around line 8-9: Update every describe and it callback in EMPTY_STATE_ROOT to
use explicit return types, annotating synchronous callbacks as (): void => and
the asynchronous callback near the end of the spec as async (): Promise<void>
=>. Preserve the existing test bodies and behavior.
---
Nitpick comments:
In `@packages/receipt-manager/src/ReceiptsManager.real.spec.ts`:
- Around line 262-274: In the test case returning logs across a block range,
update the logs[1] assertion to verify its block number separately from its
block hash. Remove the ternary expression and compare
logs[1].block.header.number directly to 2n, then compare logs[1].block.hash()
with the expected block hash so failures identify the specific mismatch.
- Around line 250-260: Update the local setupTwoBlocks helper to declare an
explicit return type describing its returned block and transaction objects,
while preserving its existing setup behavior and returned properties.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91b78e00-fa52-4594-8693-b90ead193fdb
📒 Files selected for processing (4)
packages/receipt-manager/src/ReceiptsManager.real.spec.tspackages/trie/src/EMPTY_STATE_ROOT.jspackages/trie/src/EMPTY_STATE_ROOT.spec.tspackages/trie/vitest.config.ts
| import { createMapDb } from './createMapDb.js' | ||
| import { type PostByzantiumTxReceipt, ReceiptsManager } from './ReceiptManager.js' | ||
|
|
||
| const common = optimism.copy() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether `@tevm/common`'s copy()/hardfork resolution mutates internal state
fd -t f . packages/common/src | xargs rg -n "copy\s*\(|setHardfork|_hardfork" -A5Repository: evmts/tevm
Length of output: 1582
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate files ---\n'
fd -t f 'ReceiptsManager.real.spec.ts|ReceiptManager.ts|createCommon.ts|createCommon.js' .
printf '\n--- createCommon.js relevant section ---\n'
sed -n '1,140p' packages/common/src/createCommon.js
printf '\n--- ReceiptsManager.real.spec.ts relevant sections ---\n'
sed -n '1,120p' packages/receipt-manager/src/ReceiptsManager.real.spec.ts
sed -n '230,285p' packages/receipt-manager/src/ReceiptsManager.real.spec.ts
printf '\n--- package manifests `@tevm/common` version ---\n'
rg -n '"`@tevm/common`"|"tevm-common"|"hardhat"@|hardhat@|`@nomicfoundation/hardhat`' package.json packages -g 'package.json' -g 'pnpm-lock.yaml' -g 'package-lock.json' -g 'yarn.lock' | head -200Repository: evmts/tevm
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f 'ReceiptsManager.real.spec.ts|ReceiptManager.ts|createCommon.ts|createCommon.js' .
printf '%s\n' ''
printf '%s\n' '--- createCommon.js relevant section ---'
sed -n '1,140p' packages/common/src/createCommon.js
printf '%s\n' ''
printf '%s\n' '--- ReceiptsManager.real.spec.ts relevant sections ---'
sed -n '1,120p' packages/receipt-manager/src/ReceiptsManager.real.spec.ts
sed -n '230,285p' packages/receipt-manager/src/ReceiptsManager.real.spec.ts
printf '%s\n' ''
printf '%s\n' '--- package manifests `@tevm/common` version ---'
rg -n '"`@tevm/common`"|"tevm-common"|"hardhat"@|hardhat@|`@nomicfoundation/hardhat`' package.json packages -g 'package.json' -g 'pnpm-lock.yaml' -g 'package-lock.json' -g 'yarn.lock' | head -200Repository: evmts/tevm
Length of output: 10873
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- inspect ethereumjs-common copy behavior from installed package/source if available ---'
node - <<'JS'
const fs = require('fs')
const path = require('path')
const candidates = [
'node_modules/@evmts/zevm/common',
'node_modules/.pnpm/@evmts+zevm.common*/node_modules/@evmts/zevm/common',
'../node_modules/@evmts/zevm/common',
]
for (const cand of candidates) {
const real = path.resolve(cand)
try {
const pk = require(real + '/package.json')
console.log('FOUND', real, pk.version)
const pkg = require(real)
if (typeof pkg.createCommon === 'function') {
const optimism = require('`@tevm/common`') ? require('`@tevm/common`').optimism : undefined
if (optimism) {
const c = optimism.copy()
const c2 = c.copy()
console.log('type common', typeof c)
console.log('ethjsCommon keys', Object.keys(c.ethjsCommon).sort().join(', '))
console.log('initial hardfork', c.ethjsCommon.hardfork())
console.log('initial isActivatedEIP1559', c.ethjsCommon.isActivatedEIP(1559))
const commonInstance = c.ethjsCommon
if (commonInstance.setHardfork) commonInstance.setHardfork('london')
console.log('after native-set hardfork', commonInstance.hardfork())
console.log('same object?', c2.ethjsCommon === commonInstance)
}
}
} catch (error) {
console.log('SKIP', cand, error && error.message)
}
}
JS
printf '%s\n' ''
printf '%s\n' '--- check repository source for common setHardfork methods ---'
rg -n "setHardfork|copy:\s*\(\)" packages/common/src packages -g '*.js' -g '*.ts' -g '*.tsx' -g '*.jsx' | head -200
printf '%s\n' ''
printf '%s\n' '--- full ReceiptsManager.real.spec.ts common usage locations ---'
rg -n "common\.|makeLegacyTx|make1559Tx|makeBlock|beforeEach|optimism\.copy" packages/receipt-manager/src/ReceiptsManager.real.spec.ts -A2 -B2Repository: evmts/tevm
Length of output: 10933
🌐 Web query:
@evmts/zevm common copy setHardfork ethereum-cryptography ethereumjs common copy hardfork state
💡 Result:
In the context of the Tevm (formerly EVMts) ecosystem, the Common instance is a configuration object used by the EVM and other components to manage chain-specific parameters, including network rules and hardfork states [1][2][3]. When using Tevm, you typically interact with the Common object via the createCommon function, which wraps the underlying @ethereumjs/common instance [1][2][3]. Regarding setHardfork and related configuration: 1. setHardfork (Configuration Option): In Tevm interfaces like VMOpts or BlockOptions, setHardfork is an optional boolean or BigIntLike value [4][5]. When enabled, it allows the system to set the hardfork based on a specific block number (for older hardforks) or timestamp (for hardforks from Shanghai onwards) [4][5]. It defaults to false, meaning the hardfork remains as defined by the Common instance [4][5]. 2. Common Instance Configuration: When creating a Common instance, you can explicitly set the active hardfork using the hardfork property in CommonOptions (e.g., hardfork: 'prague') [6][7]. The Common instance provides a unified view of the network and hardfork state to the EVM [1][2]. 3. Underlying EthereumJS Interaction: Tevm's Common object exposes the underlying ethereumjs Common instance via the ethjsCommon property [4][5][1]. The @ethereumjs/common library provides methods such as setHardfork(hardfork: string | null), which allows for changing the active hardfork for parameter retrieval [8]. Note that ethereumjs has refactored some of its hardfork-related methods (e.g., setHardforkByBlockNumber has been updated to more generalized setHardforkBy methods) [9]. In summary, while Common defines the initial hardfork state [2][7], setHardfork is used within Tevm execution/block options to dynamically adjust or confirm the hardfork context during operation [4][5][10].
Citations:
- 1: https://evmts.dev/reference/tevm/common/functions/createcommon/
- 2: https://github.com/evmts/tevm-monorepo/blob/main/packages/common/docs/functions/createCommon.md
- 3: https://github.com/evmts/tevm-monorepo/blob/main/packages/common/docs/README.md
- 4: https://evmts.dev/reference/tevm/vm/interfaces/vmopts/
- 5: https://evmts.dev/reference/tevm/block/interfaces/blockoptions/
- 6: https://github.com/evmts/tevm-monorepo/blob/main/packages/common/docs/type-aliases/CommonOptions.md
- 7: https://github.com/evmts/tevm-monorepo/blob/main/packages/common/src/CommonOptions.ts
- 8: https://github.com/ethereumjs/ethereumjs-common/blob/master/src/index.ts
- 9: Common: Unify and Refactor set-/getHardforkByBlockNumber ethereumjs/ethereumjs-monorepo#2798
- 10: https://github.com/evmts/tevm-monorepo/blob/main/packages/block/src/block.ts
🌐 Web query:
@ethereumjs/common copy setHardfork mutates original Common
💡 Result:
Yes, the setHardfork(hardfork) method in the @ethereumjs/common library mutates the internal state of the Common instance [1]. When you call setHardfork(hardfork) on a Common object, it updates the instance's internal _hardfork property to the specified hardfork identifier, provided that the hardfork is supported by that instance [1]. Because this method directly modifies the property on the existing instance rather than returning a new, modified clone, any other part of your application holding a reference to that same Common object will observe the updated hardfork [1]. If you need to use different hardfork settings without affecting a shared instance, you should create a copy or a new instance of the Common object [2]. For example, the @ethereumjs/block library explicitly notes that when it receives a Common object, it internally copies it to ensure that transaction behavior does not change incidentally due to future hardfork updates [2].
Citations:
- 1: https://github.com/ethereumjs/ethereumjs-common/blob/master/src/index.ts
- 2: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/block/src/types.ts
🌐 Web query:
site:github.com/ethereumjs/ethereumjs-monorepo "copy" "@ethereumjs/common" "setHardfork"
💡 Result:
In the @ethereumjs/common library, the hardfork is typically set during the instantiation of the Common class rather than via a method named setHardfork [1][2]. To set a specific hardfork, you pass it as an option to the Common constructor [1]: import { Common, Hardfork, Mainnet } from '@ethereumjs/common' // Initialize with a specific hardfork const common = new Common({ chain: Mainnet, hardfork: Hardfork.Cancun }) Additionally, note the following regarding the term "setHardfork": 1. Block and VM Options: In downstream packages like @ethereumjs/block and @ethereumjs/vm, there is a configuration option called setHardfork (usually a boolean) [3][4]. When set to true in those contexts, it instructs the library to automatically determine and set the appropriate hardfork based on the block's number or timestamp [3][4]. 2. Internal Refactoring: Historically, the library underwent internal refactoring where methods like setHardforkByBlockNumber were unified into other structures or renamed to setHardforkBy [5][6]. These are internal methods used for managing the state of a Common instance relative to chain data, rather than the primary way to define the hardfork for a new instance [5][6]. For most use cases involving setting a hardfork, you should define it in the Common constructor [1][2]. If you are working with blocks or the VM and need them to auto-detect the correct hardfork, you use the setHardfork: true option in their respective constructors or factory functions [3][4].
Citations:
- 1: https://github.com/ethereumjs/ethereumjs-monorepo/tree/master/packages/common
- 2: https://github.com/ethereumjs/ethereumjs-monorepo/releases/tag/%40ethereumjs%2Fcommon%4010.1.2
- 3: https://github.com/ethereumjs/ethereumjs-monorepo/tree/master/packages/vm
- 4: https://github.com/ethereumjs/ethereumjs-monorepo/blob/master/packages/block/src/types.ts
- 5: Block, VM: Unify hardforkBy Options ethereumjs/ethereumjs-monorepo#2800
- 6: Common: Unify and Refactor set-/getHardforkByBlockNumber ethereumjs/ethereumjs-monorepo#2798
Avoid sharing mutable Common instances across tests.
beforeEach builds chain from common.copy(), but the block and tx builders close over the top-level common / common.ethjsCommon. Since Common state can be adjusted during block/tx handling, pass each builder its own copied common too, or create the per-test common explicitly from optimism.copy() and reuse it consistently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/receipt-manager/src/ReceiptsManager.real.spec.ts` at line 11, Update
the test setup around the top-level common and the block/transaction builders so
each test uses one isolated Common instance consistently. Ensure the builders no
longer close over the shared common or common.ethjsCommon; create or reuse the
per-test optimism.copy() instance for chain construction and builder calls.
| // Non-configurable, non-writable data properties (the mutator overrides | ||
| // installed above via Object.defineProperties) must be returned as-is: | ||
| // returning a bound copy would violate the proxy invariant and throw a | ||
| // TypeError before the intended immutable/copy implementations can run. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and size =="
wc -l packages/trie/src/EMPTY_STATE_ROOT.js || true
echo "== relevant section =="
sed -n '1,140p' packages/trie/src/EMPTY_STATE_ROOT.js | cat -n
echo "== get trap occurrences in file =="
rg -n "get\\W|Object\\.define|defineProperty|defineProperties|return new Proxy" packages/trie/src/EMPTY_STATE_ROOT.js || true
echo "== surrounding files =="
git ls-files 'packages/trie/src/*' | sed -n '1,120p'Repository: evmts/tevm
Length of output: 4158
Document the new get trap with complete JSDoc.
The inline comment only records the proxy contract. Add full JSDoc for the get trap, including @param, @returns, @throws, and a working @example with imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/trie/src/EMPTY_STATE_ROOT.js` around lines 77 - 80, Add complete
JSDoc immediately above the proxy’s get trap, documenting its parameters, return
value, and possible TypeError via `@param`, `@returns`, and `@throws` tags. Include a
working `@example` with the necessary imports, while retaining the existing inline
explanation of non-configurable property handling.
Source: Coding guidelines
| const descriptor = Reflect.getOwnPropertyDescriptor(target, property) | ||
| if (descriptor !== undefined && 'value' in descriptor && descriptor.configurable === false) { | ||
| return descriptor.value | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching EMPTY_STATE_ROOT.js:"
fd -a 'EMPTY_STATE_ROOT\.js$' . || true
file="$(fd 'EMPTY_STATE_ROOT\.js$' . | head -n 1 || true)"
if [ -n "$file" ]; then
echo
echo "Line count:"
wc -l "$file"
echo
echo "Relevant section:"
sed -n '1,130p' "$file" | cat -n
fi
echo
echo "Search for EMPTY_STATE_ROOT and proxy/get-trap context:"
rg -n "EMPTY_STATE_ROOT|Reflect\.getOwnPropertyDescriptor|Proxy\(|configurable|descriptor\.writable" -S .Repository: evmts/tevm
Length of output: 14572
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
cd "$tmpdir"
node - <<'JS'
const target = {}
const writableProxy = new Proxy(target, {
defineProperty(target, property, descriptor) {
return Reflect.defineProperty(target, property, descriptor)
},
get(target, property) {
const descriptor = Reflect.getOwnPropertyDescriptor(target, property)
if (descriptor !== undefined && 'value' in descriptor && descriptor.configurable === false) {
return descriptor.value
}
const value = Reflect.get(target, property, target)
return typeof value === 'function' ? value.bind(target) : value
},
})
const nonConfigWritable = () => 'non-configurable writable'
Reflect.defineProperty(target, 'nonConfigWritable', { value: nonConfigWritable, configurable: false, writable: true })
const nonConfigNonWritable = () => 'non-configurable non-writable'
Reflect.defineProperty(target, 'nonConfigNonWritable', { value: nonConfigNonWritable, configurable: false, writable: false })
const config = () => 'configurable'
Reflect.defineProperty(target, 'config', { value: config, configurable: true, writable: true })
for (const prop of ['nonConfigWritable', 'nonConfigNonWritable', 'config']) {
const bound = new Function('return this')()
writableProxy[prop](bound)
console.log(prop, {
current: this !== bound,
nonConfigWritableCalled: prop === 'nonConfigWritable',
sameFunction: writableProxy[prop] === target[prop],
descriptor: Reflect.getOwnPropertyDescriptor(target, prop),
})
}
JSRepository: evmts/tevm
Length of output: 807
🌐 Web query:
ECMA-262 Proxy Get trap invariant non-configurable own data property
💡 Result:
Under the ECMA-262 specification, the [[Get]] trap for a Proxy exotic object must adhere to specific invariants to ensure the integrity of object property access [1][2]. When a trap is defined for the [[Get]] internal method, the Proxy object performs runtime checks on the result returned by the handler [3]. If the target object has an own data property that is non-configurable and non-writable, the [[Get]] trap invariant requires that the value returned by the trap must be the same as the [[Value]] attribute of that property's descriptor [4][1]. Specifically, if Reflect.getOwnPropertyDescriptor(target, property) returns a descriptor where [[Configurable]] is false and [[Writable]] is false, the trap must return the same value as targetDesc.[[Value]] [4][1]. If the trap returns a different value, the Proxy's [[Get]] internal method will throw a TypeError [4][1][3]. The rationale for this invariant is to maintain the reliability of property descriptors [2][3]. If a property is defined as non-configurable and non-writable, it implies that its value cannot change [3]. Without this invariant, a Proxy could report different values at different times for the same property, undermining the guarantee provided by the non-configurable, non-writable status of the property [2][3]. The specification enforces this by having the Proxy object's [[Get]] internal method check the trap's result against the target's actual property descriptor, throwing an error if they conflict [1][3].
Citations:
- 1: https://read262.netlify.app/ordinary-and-exotic-objects-behaviours/proxy-object-internal-methods-and-internal-slots/
- 2: https://es.discourse.group/t/what-is-the-rationale-behind-the-invariants-of-the-get-internal-method-of-proxy-exotic-objects/1847
- 3: Something about Invariants in specification (needs clarification). tc39/ecma262#1628
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/Proxy/get
Restrict the invariant path to non-writable data properties.
The current descriptor fast path also bypasses binding for non-configurable writable function properties, even though this proxy can define such non-index properties. Those functions escape unbound and can fail when this is detached, e.g. in an async callback. Add descriptor.writable === false to match the non-configurable/non-writable invariant.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/trie/src/EMPTY_STATE_ROOT.js` around lines 81 - 84, Update the
descriptor fast path in the proxy get logic to return descriptor.value only when
the own data property is both non-configurable and non-writable, adding the
descriptor.writable check while preserving existing behavior for other
properties.
| describe('EMPTY_STATE_ROOT', () => { | ||
| it('is the well-known keccak256(rlp(empty)) state root', () => { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file exists and size =="
wc -l packages/trie/src/EMPTY_STATE_ROOT.spec.ts || true
echo "== file contents =="
cat -n packages/trie/src/EMPTY_STATE_ROOT.spec.ts
echo "== nearby files / lint config hints =="
git ls-files | rg '(^|/)(biome\.jsonc?|eslint\.|vitest\.|package\.json|pnpm-lock\.yaml|yarn\.lock|package-lock\.json)$' | sed -n '1,80p'Repository: evmts/tevm
Length of output: 8863
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== root biome.json =="
cat -n biome.json | sed -n '1,220p'
echo "== tsconfig snippets mentioning noInferFunctionReturnType / explicitFunctionReturnType / strict =="
for f in $(git ls-files | rg '(^|/)tsconfig.*\.json$|(^|/)biome\.json$'); do
echo "-- $f --"
rg -n "noInfer|explicitFunctionReturnType|function|return|strict" "$f" || true
done
echo "== vitest/test files returning callbacks with/without explicit types =="
rg -n "it\(['\"][^'\"]+['\"]\)\s*=>|describe\(['\"][^'\"]+['\"]\)\s*=>|it\(['\"][^'\"]+['\"]\),?\s*$|describe\(['\"][^'\"]+['\"]\),?\s*$" packages/trie/src/EMPTY_STATE_ROOT.spec.tsRepository: evmts/tevm
Length of output: 9640
Add explicit return types to the test callbacks.
The callbacks in this spec all rely on inferred returns. Annotate each with (): void =>, and the test at line 90 as async (): Promise<void> =>, to keep the file aligned with the explicit return-type guideline for **/*.{ts,tsx}.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/trie/src/EMPTY_STATE_ROOT.spec.ts` around lines 8 - 9, Update every
describe and it callback in EMPTY_STATE_ROOT to use explicit return types,
annotating synchronous callbacks as (): void => and the asynchronous callback
near the end of the spec as async (): Promise<void> =>. Preserve the existing
test bodies and behavior.
Source: Coding guidelines
- genesisStateRoot takes GenesisState, whose values are `0x${string}` | AccountState.
The state literal widened to `string`, so it needed `as const`.
- The `delete EMPTY_STATE_ROOT[0]` line did not actually produce a type error, so the
@ts-expect-error directive was itself an error (TS2578). Removed.
Verified: tsc --noEmit clean, 9/9 tests pass.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
1 participant
Motivation
The suite starts an
anvilchild process for essentially every stateful test, behind a Prool proxy on a fixed port. That means a Foundry binary onPATH, a process per worker, port bookkeeping, and startup polling before a single assertion runs.Tevm is an EVM that runs inside the Vitest worker. This PR wires it up as a first-class test lane and migrates as much of the suite to it as rc.151 correctly supports — while showing off what the Tevm test library can actually do.
Before / after
anvilchild processMemoryClientcreateMemoryClient()returns immediately;tevmReady()awaits the fork anchorevm_snapshotbaseline, reverted before every testsetup.global.tsnoMining: trueminingConfig: { type: 'manual' }Viem clients are still constructed with this repository's
createClient, over acustomEIP-1193 transport that forwards tomemoryClient.request. Migrated suites therefore still exercise this fork's actions, client, and transport code — Tevm only supplies the node. Tevm depends on a newer published Viem than this fork builds; that boundary is crossed in exactly one place, the adapter intest/src/tevm.ts.What's here
A third Vitest project.
tevmmatchessrc/**/*.tevm.test.ts, usestest/setup.tevm.ts, and has noglobalSetup. Everything it shares withcoremoved totest/setup.shared.ts.test/src/tevm.ts—tevmMainnet, the Tevm counterpart ofanvilMainnet. Pinned mainnet fork, manual mining, the familiar prefunded accounts,getClient()with the same shape as the Anvil fixture, and snapshot-based per-test isolation (a successfulevm_revertconsumes its id, so a replacement snapshot is taken immediately).17 migrated action suites —
getCode,getStorageAt,dropTransaction,dumpState,impersonateAccount,increaseTime,mine,revert,setBalance,setCode,setNextBlockTimestamp,setNonce,setStorageAt,snapshot,stopImpersonatingAccount,setBlockTimestampInterval,removeBlockTimestampInterval.The complete HTTP transport suite.
test/src/tevm-server.tsserves aMemoryClientover an ephemeral127.0.0.1listener (port0) usingcreateServerfromtevm/serverfor the real-node paths;test/src/http-server.tsis a plainnode:httpserver for the JSON-RPC error and header cases, whichtevm/serveranswers with HTTP 400 rather than 200-with-error. No coverage was dropped.Tevm test features shown off
src/tevm-showcase.tevm.test.ts, split into focused cases:MemoryClientand this repo's Viem client in one process; manual mining (transactions stay pending untiltevmMine); snapshot & revert through the Anvil-compatibleanvil_snapshot/anvil_revertRPC surfacetevmSetAccount,tevmDeal,tevmDeploy,tevmContract,tevmMine,impersonateAccountTevmNodehandlers —callHandler,contractHandler,dealHandler,deployHandler,setAccountHandleragainst a barecreateTevmNode()SimpleContract,AdvancedContract,ErrorContract,TestERC20from@tevm/test-utils@tevm/test-matchersfamily — primitives (toBeAddress,toBeHex,toEqualAddress,toEqualHex), account & state (toBeInitializedAccount,toHaveState,toHaveStorageAt), events (toEmitwithwithEventArgs/withEventNamedArgs), traces (toCallContractFunctionwithwithFunctionNamedArgs), reverts (toBeRevertedWithString,toBeRevertedWithErrorwithwithErrorNamedArgs), balances (toChangeBalance,toChangeTokenBalance)Honesty about rc.151
Several matcher families run against non-fork
createTevmNode()instances: on a forked node rc.151 resolves balances and storage througheth_getProofagainst the upstream historical block, so locally written values are invisible.test/README.mdlists every gap found.Six suites keep their original Anvil coverage and gain a
.tevmsibling that asserts Tevm's actual behaviour rather than restating Anvil's — nothing is weakened:.tevmsibling documentssetStorageAteth_getStorageAtreturns the storage word right-paddedgetStorageAtimpersonateAccount/stopImpersonatingAccountsetBlockTimestampInterval/removeBlockTimestampIntervalanvil_setBlockTimestampIntervalis recorded but not applied to mined blocksThe fork is pinned to 22263621, the nearest blob-free block below Anvil's 22263623: rc.151's
Common.copy()dropscustomCrypto, so a forked anchor containing EIP-4844 transactions cannot be deserialized. Choosing a blob-free anchor is preferred over filtering type-3 transactions out of an otherwise inconsistent block response — the fork sees exactly what the provider returns.Anvil is not going anywhere: WebSocket and IPC transports, the Alto bundler / account-abstraction suites, the Optimism / zkSync / Sepolia fixtures, wallet-provider emulation,
sendUnsignedTransaction,setAutomineandsetIntervalMiningall still need it, andpnpm installstill runscontracts:buildwithforge.Dependency hygiene
Tevm packages are pinned to exact
1.0.0-rc.151(the movinglatest/nexttags resolve to older builds with a much smaller Anvil surface), andpnpm.packageExtensionspins the entire Tevm family onto one published Viem —@tevm/errorswas resolving a second copy and is now pinned too.@tevm/serveris deliberately not installed (itslatesttag is an obsolete0.0.1);createServercomes fromtevm/server.@tevm/ts-pluginis not a dependency until direct Solidity imports are actually enabled.src/tevm-resolution.tevm.test.tsguards all of that at runtime: it imports every Tevm entrypoint the suite uses, so a peer-resolution regression — Tevm silently binding the unbuilt workspace Viem — fails here, first and legibly, instead of as an opaque module error deep inside an unrelated suite.No changeset: this change is entirely test infrastructure and
devDependencies, with no effect on the published package.Test evidence
The Tevm lane, against an archive mainnet RPC:
The six suites whose original Anvil coverage was restored, on the
core(Anvil) project:biome check src testreports no new findings (the 9 warnings are pre-existing, all insrc/tempo/).tsc -breports no errors in any file this PR adds or modifies; the remaining errors are the pre-existing Voltaire-migration ones onmain.Credits
Produced by a multi-agent run: three independent implementation lanes (
codex-sol,opus,kimi) were built and judged, and this branch is the winning lane with the best ideas from the others grafted in — the focused showcase split, the explicit rc.151 limitations matrix and retained-Anvil coverage, the blob-free fork block, dropping@tevm/ts-plugin, and the runtime resolution smoke tests.🤖 Generated with Smithers multi-agent orchestration
Summary by CodeRabbit
Bug Fixes
Tests