fix(evm): read operation inputs when they are submitted - #382
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
commit: |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e16458e4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const merged = merge(evm['~config'].block, block) | ||
| return attempt(evm, () => apply(evm, { ...evm['~config'], block: merged })) |
There was a problem hiding this comment.
When an asynchronous EVM receives multiple setters without awaiting each one, every merged value is based on the same pre-queue configuration. For example, queuing setBlock({ number: 5n }) followed by setBlock({ timestamp: 10n }) causes the second operation to restore the old block number, even though omitted fields are meant to remain unchanged. Snapshot the supplied partial block at submission, but merge it with the current block inside the queued attempt; setBlockAndExecutionConfig has the same eager-merge problem.
Useful? React with 👍 / 👎.
| const request = { | ||
| envelope: envelope(transaction, evm['~chainId']), | ||
| signer: transaction.from, |
There was a problem hiding this comment.
When transaction.serialized is a Uint8Array, envelope() calls Bytes.from, which returns that same array rather than copying it. Consequently, a caller mutating the serialized buffer after callTx or transact returns—but before the asynchronous queue runs or while a database read is pending—still changes the submitted transaction despite this new eager request construction. Copy the resulting envelope bytes before retaining them in the queued request.
Useful? React with 👍 / 👎.
| balanceChanges: [...account.balanceChanges], | ||
| codeChanges: [...account.codeChanges], | ||
| nonceChanges: [...account.nonceChanges], | ||
| storageChanges: account.storageChanges.map((slot) => ({ | ||
| ...slot, | ||
| changes: [...slot.changes], |
There was a problem hiding this comment.
For a queued setBal, the new snapshot copies the change arrays but retains each mutable entry object. If the caller subsequently modifies values such as balanceChanges[0].balance, nonceChanges[0].nonce, or storageChanges[0].changes[0].value, encoding later observes the mutation rather than the values submitted to setBal. Clone the individual change entries as well as their containing arrays.
Useful? React with 👍 / 👎.
| TxEnvelope.from({ | ||
| ...rest, | ||
| chainId: (rest as { chainId?: number }).chainId ?? Number(chainId), |
There was a problem hiding this comment.
When a fields transaction carries chainId: null, which the loose index-signature input type permits, the new nullish fallback silently replaces it with the EVM chain ID. Previously the null reached envelope validation and was rejected, so malformed or incompletely normalized RPC input can now execute on a different chain context instead of failing. Only fall back when the property is strictly undefined.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| describe('fields', () => { | ||
| test('behavior: an explicit undefined chain id falls back to the EVM default', async () => { |
There was a problem hiding this comment.
This new file exercises Evm.callTx, but its cases are grouped under describe('fields') and describe('blob transactions'); similarly, snapshot.test.ts combines several exported functions under describe('queued operations'). Split these into describe('callTx', ...), describe('setBlock', ...), and the other targeted function blocks so failures follow the repository's required test organization.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| const request = { | ||
| envelope: envelope(transaction, evm['~chainId']), |
There was a problem hiding this comment.
For an asynchronous EVM, callTx and transact are typed to return promises, but moving envelope() before attempt() means normal envelope validation failures now throw synchronously instead of rejecting those promises. Code such as Evm.callTx(fork, invalid).catch(...) therefore crashes before obtaining the promised value, whereas the previous queued encoding produced a rejection. Catch eager encoding errors and return a rejected promise when ~driver is present, while retaining synchronous throws for synchronous EVMs.
Useful? React with 👍 / 👎.
| const executed = await queued | ||
|
|
||
| // Targeted the original address, not the mutated one. | ||
| expect(ExecutedTx.result(executed).status).toBe(true) |
There was a problem hiding this comment.
This assertion does not prove that the original address was used: the original contract completes successfully, but a system call to the mutated, nonexistent 0xff account also completes successfully as an empty-code call. The test therefore passes even if systemCall still reads options.address when the queue drains; make the two targets return observably different output or commit and inspect the original target's storage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4f7219986
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| }) | ||
| }) | ||
|
|
||
| describe('caller-held state', () => { |
There was a problem hiding this comment.
This block combines tests for PendingState.accountInfo and PendingState.insertStorage under a topic-based name, so failures are not organized by the exported function they exercise. Split these cases into describe('accountInfo', ...) and describe('insertStorage', ...) blocks as required by the repository test convention.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| }) | ||
| }) | ||
|
|
||
| describe('self-destruct', () => { |
There was a problem hiding this comment.
This case exercises Evm.commitSource, but the topic-based describe('self-destruct') leaves it outside the file's existing describe('commitSource', ...) group. Move it into that exported-function block so the suite follows the required test organization.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
| ): Awaitable<boolean, TxResult.TxResult> { | ||
| // Encoded once: a retry replays the submitted transaction, not whatever the | ||
| // caller's object became while an uncached read was fetched. | ||
| const request = { |
There was a problem hiding this comment.
This changes the observable behavior of public EVM operations by snapshotting caller inputs at submission, but the commit does not update any changeset. Because this branch already has changesets for call execution, transaction lifecycle, configuration, inspection, and block access lists, update the relevant existing entries rather than adding a duplicate so the release records these semantics.
AGENTS.md reference: AGENTS.md:L118-L120
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: feb4864538
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (block['~engine'] !== evm['~engine']) throw new NoBlockStateError() | ||
| return evm['~engine'].takeBlockState(block['~id']) |
There was a problem hiding this comment.
On an asynchronous EVM, this callback runs in a later queue microtask, so JavaScript callers can mutate the newly object-shaped token after takeBlockState returns but before these properties are read. Changing ~id can make the operation reject for a valid accumulator—or target another generation—rather than consuming the token that was submitted; capture ~engine and ~id before calling attempt, or freeze tokens when creating them.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| const { sidecars: _sidecars, ...rest } = fields as typeof fields & { | ||
| sidecars?: unknown | ||
| } |
There was a problem hiding this comment.
When sidecars is the only EIP-4844 discriminator on a loose fields input, removing it before TxEnvelope.from changes the inferred envelope type to EIP-1559. Such an input previously followed the EIP-4844 path and rejected the missing blobVersionedHashes, but now it can execute as an unrelated type-2 transaction; infer the envelope type before discarding the network wrapper, or retain type: 'eip4844' when sidecars selected it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b48e64502
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ]), | ||
| ), | ||
| [notice]: attribution(revision, compiled.metadata), | ||
| [thirdParty]: thirdPartyLicenses(compiled.metadata, compiled.texts), |
There was a problem hiding this comment.
When OX_EVM2_NATIVE=1 selects the documented host-build path, compile() returns no texts property, but this unconditional call passes compiled.texts to thirdPartyLicenses, which immediately calls texts.get(...). The native build therefore crashes after completing the expensive Rust/WASM compilation instead of producing its outputs; collect the host Cargo-cache license texts in that branch and include them in Compiled.
AGENTS.md reference: AGENTS.md:L138-L138
Useful? React with 👍 / 👎.
| 'mkdir -p /out/licenses', | ||
| 'for dir in "$CARGO_HOME"/registry/src/*/*/ "$CARGO_HOME"/git/checkouts/*/*/; do' + | ||
| ' [ -d "$dir" ] || continue;' + | ||
| ' name=$(basename "$dir");' + |
There was a problem hiding this comment.
For git dependencies, Cargo stores files under git/checkouts/<repository-hash>/<revision>/, so this basename is a revision rather than the ${entry.name}-${entry.version} key used by thirdPartyLicenses. Consequently, license files copied from git checkouts are never matched to their packages; the generated file already reports the six linked ark-* git crates as having no license file even though their checkout contains repository-level licenses. Resolve git checkout texts to the linked package keys before generating the distributable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52e17ce018
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Ok(Bytecode::new_raw_checked(bytes.clone()) | ||
| .unwrap_or_else(|_| Bytecode::new_legacy(bytes))) |
There was a problem hiding this comment.
When state contains code beginning with 0xef01 that is not a 23-byte delegation designator, this fallback now executes it as legacy for a custom or asynchronous Database, while Database.fromMemory still rejects the exact same bytes through assertCode in src/evm/Database.ts:199-206. Replacing an equivalent database implementation can therefore change whether the state executes at all; preserve new_raw_checked rejection or update every database path consistently after recording and approving the evm2 divergence.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c550851312
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| records: [ | ||
| ...changes.records.filter( | ||
| (record) => | ||
| !( | ||
| (record.kind === 'storage' || record.kind === 'storageRead') && | ||
| matches(record) | ||
| ), | ||
| ), | ||
| { ...slot, kind: 'storage' as const }, |
There was a problem hiding this comment.
When a detached state contains trailing accountRead or storageRead records, replacing an existing slot removes its original record and appends the new write after those reads. StateChange.visit therefore emits an order that evm2's pending-state visitor would not produce, which can change the behavior of order-sensitive persistence or audit sinks; replace the matching record in place or rebuild the canonical evm2 order instead. insertAccount uses the same append pattern.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| // A crate shipping no license file is named anyway, so a gap is visible | ||
| // rather than silently absent. | ||
| return text | ||
| ? `${heading}\n\n${text}` | ||
| : `${heading}\n\nNo license file ships with this crate; see its repository.` |
There was a problem hiding this comment.
When publishing the package, this fallback leaves required attribution unresolved even though the generated file says every linked crate's own license text is reproduced. Fresh evidence in the regenerated artifact shows twelve linked crates still taking this path, including alloy-consensus, alloy-primitives, ruint, and valuable; resolve repository/workspace-level license files or otherwise provide their distributable notices rather than shipping “see its repository.”
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 844d1b59cd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --- | ||
|
|
||
| Added `ox/evm`, an EVM backed by `alloy-rs/evm2` compiled to WebAssembly, with read-only transaction execution. | ||
| Added `ox/evm`, an EVM backed by `alloy-rs/evm2` compiled to WebAssembly, with read-only transaction execution; every operation reads its inputs when it is submitted, so mutating them afterwards cannot change what runs. |
There was a problem hiding this comment.
With an asynchronous database, this new guarantee is still false for Evm.commitSource: src/evm/Evm.ts:1253-1255 calls PendingState.changes(state) only when the queued callback runs, while PendingState.insertAccount retains the caller's mutable current and original objects. Mutating either object after commitSource returns but before the queue drains silently changes the state that gets applied; capture a deep snapshot at submission as the other state-bearing operations now do.
AGENTS.md reference: AGENTS.md:L49-L49
Useful? React with 👍 / 👎.
| } | ||
| } | ||
|
|
||
| test('visit replays the order a streamed resolution observes', async () => { |
There was a problem hiding this comment.
Fresh evidence in the final tree is that this StateChange.visit case remains a top-level test instead of living under describe('visit', ...); src/evm/_test/borrow.test.ts:45 likewise leaves its callTx behavior at the top level. Wrap these cases in blocks named for the exported functions they exercise so the new suites follow the repository's required organization.
AGENTS.md reference: AGENTS.md:L92-L92
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78db9786e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| expect(Evm.callTx(twice, transaction())).toEqual(expected) | ||
| }) | ||
|
|
||
| test('behavior: a token from another EVM is refused', async () => { |
There was a problem hiding this comment.
This cross-EVM case exercises Evm.takeBlockState, but it is nested inside describe('warmPrecompiles'), so failures are attributed to an unrelated export. Move it into a describe('takeBlockState', ...) block to follow the required per-export test organization.
AGENTS.md reference: AGENTS.md:L87-L92
Useful? React with 👍 / 👎.
1 participant
Addresses the review findings on #366: operation inputs are read when submitted rather than when they run, envelopes are built from fields a caller can actually supply, caller-held state is copied at the boundary, and the artifact ships each linked crate's license text.
Five claims did not hold and are covered by regression tests instead: self-destruct wipes, bigint chain IDs, and the detached visit order.