Skip to content

Fix invalid eth_simulate schema #675

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+20 −13
Open

Fix invalid eth_simulate schema #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f3b589f
eth: fix invalid eth simulate schema
lightclient Jul 16, 2025
b6d9c44
eth: add back err message for call status
lightclient Jul 16, 2025
c56ee82
schemas: fixup based on pr comments
lightclient Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/schemas/block.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ Block:
- size
- transactions
- uncles
additionalProperties: false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like this might be problematic, going forward. I think I see why you want, it's because you want to reuse the block schema with the additional property calls, without specifying an additional schema for Block.

But for more clarity we don't support later version of the json draft spec yet officially. So technically unevaluatedProperties is not supported.

I peeked at the problem, and it seemed as though the issue with the original PR that landed was that calls, was set as required, but not defined in properties which, was one of the reasons why build broke

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, you think removing additionalProperties is problematic or you think keeping it is?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually sorry, misread the additionalProperties as being additionalProperties:True. The additionalProperties:False should be fine.

unevaluatedProperties portion is not officially supported so if it's possible to rm that I would probably rm that, until the open-rpc spec supports a later json schema draft spec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay gotcha. Yeah agree we should remove the unevaluatedProperties.

properties:
hash:
title: Hash
Expand Down
28 changes: 20 additions & 8 deletions src/schemas/execute.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ Withdrawals:
description: This array can have a maximum length of 16.
type: array
items:
- $ref: '#/components/schemas/Withdrawal'
$ref: '#/components/schemas/Withdrawal'
Withdrawal:
title: A withdrawal made by a validator
type: object
Expand All @@ -168,7 +168,7 @@ Withdrawal:
$ref: '#/components/schemas/uint64'
EthSimulateResult:
title: Full results of multi call
type: object
type: array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an object. See the full proper fix for this issue in https://github.com/ethereum/execution-apis/pull/670/files, which we were waiting to merge until the CI bug was fixed but we could merge that now if we don't want to wait for the bug in the CI checker to get fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If #670 is the proper fix and it should be an object, then the tests need to be fixed because it currently returns an array. See the result here: https://github.com/ethereum/execution-apis/blob/main/tests/eth_simulateV1/ethSimulate-set-read-storage.io

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I think I see the problem. This is a oneOf and one of the options is an array and the other is an object. I'm not sure how to capture this in openrpc specs. Do we just remove the type entirely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the object EthSimulateBlockResultInvalid is also wrong, but there are no tests for it so it doesn't fail the schema. AFAICT, invalid results fail with a JSON-RPC error, not a success response with result field.

Not sure what the expected value for it is.

oneOf:
- $ref: '#/components/schemas/EthSimulateBlockResultInvalid'
- $ref: '#/components/schemas/EthSimulateBlockResultSuccess'
Expand All @@ -182,7 +182,8 @@ EthSimulateBlockResultSingleSuccess:
type: object
allOf:
- $ref: '#/components/schemas/Block'
- title: Eth Simulate call results
- type: object
title: Eth Simulate call results
required:
- calls
properties:
Expand Down Expand Up @@ -265,10 +266,21 @@ CallResultFailure:
$ref: '#/components/schemas/uint64'
error:
oneOf:
- code: -32000
message: 'Execution reverted'
- code: -32015
message: 'VM execution error'
- type: object
properties:
code:
const: -32000
message:
type: string
pattern: "^execution reverted.*"
required: [code, message]
- type: object
properties:
code:
const: -32015
message:
type: string
pattern: "^vm execution error.*"
Comment on lines +275 to +283
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does case matter here? I haven't tested, but I thought that these were both capital first letter casing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could make the regex support both cases, but currently the response is lowercase. See the test here: https://github.com/ethereum/execution-apis/blob/main/tests/eth_simulateV1/ethSimulate-eth-send-should-not-produce-logs-on-revert.io

CallResultSuccess:
title: Result of call success
type: object
Expand Down Expand Up @@ -298,7 +310,7 @@ CallResultLog:
type: object
required:
- logIndex
- blockhash
- blockHash
- blockNumber
- transactionHash
- transactionIndex
Expand Down
1 change: 0 additions & 1 deletion src/schemas/transaction.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,6 @@ TransactionInfo:
- from
- hash
- transactionIndex
unevaluatedProperties: false
properties:
blockHash:
title: block hash
Expand Down
3 changes: 0 additions & 3 deletions tests/eth_simulateV1/ethSimulate-empty-ethSimulate.io
View file
Open in desktop

This file was deleted.