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

JSON schema for state tests #213

Merged
merged 19 commits into from Aug 22, 2017

Conversation

Projects
None yet
4 participants
@jwasinger
Member

jwasinger commented Jul 6, 2017

I'm opening this pull request for feedback on what I have implemented so far regarding #204

I didn't originally intend to write the schema validation script in JavaScript. Unfortunately, the only library I could find that supported limiting allowed JSON object properties via "additionalProperties": "false" was implemented using Node.

@jwasinger jwasinger changed the title from Issue 204 to JSON schema for state tests Jul 6, 2017

@sdtsui

@jwasinger - thanks for the contribution.

Looks like a great start.

Looking at your failing tests, are there some conditions the schema has to account for, that aren't explicitly in the EIP but needed for state tests to pass?

cc/ @cdetrio.

For example:

GeneralStateTests/stTransactionTest/zeroSigTransacrionCreate.json:
   instance.zeroSigTransacrionCreate.transaction.to does not match pattern "^0x[0-9a-f]*$"
   instance.zeroSigTransacrionCreate.transaction additionalProperty "r" exists in instance when not allowed
   instance.zeroSigTransacrionCreate.transaction additionalProperty "s" exists in instance when not allowed
   instance.zeroSigTransacrionCreate.transaction additionalProperty "v" exists in instance when not allowed

The extra props can be read about here. There are others.

Show outdated Hide outdated JSONSchema/schema.json Outdated
Show outdated Hide outdated JSONSchema/schema.json Outdated
Show outdated Hide outdated JSONSchema/schema.json Outdated
@cdetrio

This comment has been minimized.

Show comment
Hide comment
@cdetrio

cdetrio Jul 6, 2017

Member

Great!

Member

cdetrio commented Jul 6, 2017

Great!

Show outdated Hide outdated JSONSchema/schema.json Outdated
@jwasinger

This comment has been minimized.

Show comment
Hide comment
@jwasinger

jwasinger Jul 9, 2017

Member

@cdetrio I think this is almost ready to be merged. I just want to add some more linting around allowed hex values for certain fields first (e.g. a byte field cannot have a string of more than length 2, an address cannot be more than length 40, etc.). I'm guessing this will probably reveal some additional flawed tests.

I'll try to have these changes ready within a few hours.

Member

jwasinger commented Jul 9, 2017

@cdetrio I think this is almost ready to be merged. I just want to add some more linting around allowed hex values for certain fields first (e.g. a byte field cannot have a string of more than length 2, an address cannot be more than length 40, etc.). I'm guessing this will probably reveal some additional flawed tests.

I'll try to have these changes ready within a few hours.

@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Jul 26, 2017

Member

Shall we fix the Travis build before we merge?

Member

pirapira commented Jul 26, 2017

Shall we fix the Travis build before we merge?

@cdetrio

This comment has been minimized.

Show comment
Hide comment
@cdetrio

cdetrio Aug 5, 2017

Member

Added a schema for state test fillers. The schemas are quite permissive, only 15 state tests are failing now, all because their transaction.data fields are missing a 0x prefix. The corresponding filler files are also failing for the same reason.

If the 0x prefix is added to the failing files, and #250 is merged (with a couple other minor fixes), then the travis build should be green. At least then the formats shouldn't get any worse.

The schemas can be gradually made more strict, and the files fixed over time. Or alternatively, we can add a rule in the schema to detect newly generated files, and enforce a strict schema on those.

Member

cdetrio commented Aug 5, 2017

Added a schema for state test fillers. The schemas are quite permissive, only 15 state tests are failing now, all because their transaction.data fields are missing a 0x prefix. The corresponding filler files are also failing for the same reason.

If the 0x prefix is added to the failing files, and #250 is merged (with a couple other minor fixes), then the travis build should be green. At least then the formats shouldn't get any worse.

The schemas can be gradually made more strict, and the files fixed over time. Or alternatively, we can add a rule in the schema to detect newly generated files, and enforce a strict schema on those.

@pirapira pirapira merged commit 3c8e613 into ethereum:develop Aug 22, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pirapira

This comment has been minimized.

Show comment
Hide comment
@pirapira

pirapira Aug 22, 2017

Member

Merging while it's passing.

Member

pirapira commented Aug 22, 2017

Merging while it's passing.

@jwasinger jwasinger deleted the jwasinger:issue-204 branch Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment