Fix invalid eth_simulate schema #675
Conversation
There was a problem hiding this comment.
The blocker for us merging https://github.com/ethereum/execution-apis/pull/670/files is that CI currently appears to be broken and the checker appears to be checking main when run on PR rather than checking the PR. This means any change that breaks CI will pass until after it is merged, and then everything will fail (including PRs that fix the bug like #670) until main has a fix.
We can ignore this and merge #670 so the checker stops failing for every PR, but we were hoping to wait until CI was fixed so our PR could be properly checked before merging.
That being said, this PR is somehow passing the checker, which surprises me and I'm not sure why that would be. Perhaps because the PR was authored by a contributor while #670 is authored by a non-contributor and permissions on the run are causing it to run differently?
Either way, I recommend merging #670 prior to this, and removing the changes related to eth_simulateV1 from this PR.
| pattern: "^execution reverted.*" | ||
| required: [code, message] | ||
| - type: object | ||
| properties: | ||
| code: | ||
| const: -32015 | ||
| message: | ||
| type: string | ||
| pattern: "^vm execution error.*" |
There was a problem hiding this comment.
Does case matter here? I haven't tested, but I thought that these were both capital first letter casing?
There was a problem hiding this comment.
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
| type: object | ||
| type: array |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -20,7 +20,6 @@ Block: | |||
| - size | |||
| - transactions | |||
| - uncles | |||
| additionalProperties: false | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
To clarify, you think removing additionalProperties is problematic or you think keeping it is?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Okay gotcha. Yeah agree we should remove the unevaluatedProperties.
|
@MicahZoltu I don't think there are different permissions for running the PRs. When I verify if #670 passes Obviously you and the other people working on My preference would be to merge this so that CI is no longer broken, then separately if you want to change the implementation of eth_simulate to be more aligned with the original idea of the spec, that can be done. Otherwise our options are to revert the PR that merged eth_simulate and wait until the spec and tests are in harmony, or we if #670 can be fixed ASAP we could merge it instead. |
|
#484 was passing all checks at the time of merge. However, it does appear that it is correctly checking now. I propose you remove the That being said, I think both PRs have #675 (comment) wrong, which I don't know how to resolve (spec format question). |
|
Unfortunately in #484 it seems since @KillariDev hadn't previously landed a commit, so the CI tests were not running and only this GitGuardian action was running. I guess Felix didn't notice this before merging. Let me try to bring the changes from #670 here and then we can figure out deal with the types. |
Fixes the issues that was causing CI to break. I see there are still some more issues in the same vein of what was fixed (particularly around the constant error codes), but will probably resolve those in a separate PR.