Skip to content
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.

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

add additionalProperties: false to objects #435

Merged
merged 8 commits into from
Jul 12, 2023

Conversation

anukul
Copy link
Contributor

@anukul anukul commented Jul 7, 2023

Closes #63

@anukul
Copy link
Contributor Author

anukul commented Jul 7, 2023

Seeing a speccheck failure for get-block-by-hash response after this - https://www.jsonschemavalidator.net/s/60jyjLPr

Can discuss specific failures here if the test workflow can be approved please

scripts/build.js Outdated Show resolved Hide resolved
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Hey @anukul thanks for working on this! Would love to see this merged. The issue is a bit deceiving though I think, because the task is a bit more difficult than simply marking every method additionalProperties: false.

When this is marked on the schema and additional values are disallowed, many test values stop validating against the spec. You can see in the CI result for speccheck. The output could use some work, but last two lines are the important ones. The last line is the json value that failed validation and the second to last line is the schema it failed to validate against.

Here is what it is currently failing on: https://www.jsonschemavalidator.net/s/VPmC37j8

It appears to be an issue with how transactions are represented within blocks. There are extra fields like blockHash, blockNumber, etc. These fields are defined in TransactionInfo, but not SignedTransaction, which is what Block is currently using. I think it should be using TransactionInfo instead.

Hopefully this is helpful - let me know if you have any questions. The bulk of the work for resolving this issue will be working though the speccheck errors and updating the schema so that it matches exactly the test cases.

scripts/build.js Outdated Show resolved Hide resolved
scripts/build.js Outdated Show resolved Hide resolved
@anukul
Copy link
Contributor Author

anukul commented Jul 9, 2023

The issue is a bit deceiving though I think, because the task is a bit more difficult than simply marking every method additionalProperties: false.

Right, thanks a lot for reviewing.

After changing Block.transactions to TransactionInfo, I noticed that setting additionalProperties: false in a child schemas (c1, c2) causes certain types of composition to fail ({ oneOf: [c1, c2], properties: { ... } }

I took the approach of setting it everywhere and running speccheck + eyeballing for other such tricky instances.

Also attempted to fix two other reported errors:

  • Missing property block.hash
  • Missing property tx_receipt.type

@anukul anukul requested a review from lightclient July 9, 2023 05:52
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Looking good, just a few comments.

src/engine/openrpc/methods/payload.yaml Outdated Show resolved Hide resolved
src/schemas/transaction.yaml Show resolved Hide resolved
@anukul anukul requested a review from lightclient July 11, 2023 01:52
Copy link
Member

@lightclient lightclient left a comment

Choose a reason for hiding this comment

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

Great work! Glad to finally close this issue.

@lightclient lightclient merged commit 8fcafbb into ethereum:main Jul 12, 2023
@anukul anukul deleted the additional-properties branch July 12, 2023 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set additionalProperties to false
2 participants