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

Fix typed message schema mistake #243

Merged
merged 3 commits into from
Apr 21, 2022
Merged

Fix typed message schema mistake #243

merged 3 commits into from
Apr 21, 2022

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 19, 2022

The exported JSON schema TYPED_MESSAGE_SCHEMA was changed in v4 to specify all supported Solidity types, but this accidentally prevented the use of reference types.

This change was undone, so that the schema no longer validates that the types used are valid. I could not find a way to preserve this validation without disallowing reference types. JSON schema did not make this possible.

Tests were added for the typed message schema to ensure this doesn't happen again.

Fixes #242

The exported JSON schema `TYPED_MESSAGE_SCHEMA` was changed in v4 to
specify all supported Solidity types, but this accidentally prevented
the use of reference types.

This change was undone, so that the schema no longer validates that
the types used are valid. I could not find a way to preserve this
validation without disallowing reference types. JSON schema did not
make this possible.

Tests were added for the typed message schema to ensure this doesn't
happen again.

Fixes #242
@Gudahtt Gudahtt requested a review from a team as a code owner April 19, 2022 20:49
mcmire
mcmire previously approved these changes Apr 20, 2022
Copy link

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

That's very interesting that JSON Schema didn't allow you to do what you wanted to do.

A couple of nits below but looks good to me regardless.

describe('TYPED_MESSAGE_SCHEMA', () => {
it('should match valid typed message', () => {
const ajv = new Ajv();
const validate = ajv.compile(TYPED_MESSAGE_SCHEMA);
Copy link

Choose a reason for hiding this comment

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

[nit] What do you think about extracting this validate function since it's the same for every test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea! Done in b5f0d87

});

for (const solidityType of eip712SolidityTypes) {
// eslint-disable-next-line no-loop-func
Copy link

Choose a reason for hiding this comment

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

[nit] If you use .forEach instead of a for loop, does it remove the need for this override?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does, but now that you've brought this up, I kinda want to get to the bottom of this. This ought to work. Something is messed up with the ESLint globals configuration. I'll look into this in a subsequent PR.

Copy link
Member Author

@Gudahtt Gudahtt Apr 20, 2022

Choose a reason for hiding this comment

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

Oh my goodness. I found the problem. The override for the test ESLint config is wrong, the file glob uses the js file extension rather than ts. There are a bunch of violations, I'll fix this separately.

Edit: Done in #245

@Gudahtt Gudahtt merged commit 7d4e621 into main Apr 21, 2022
@Gudahtt Gudahtt deleted the fix-typed-message-schema branch April 21, 2022 20:46
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.

TYPED_MESSAGE_SCHEMA does not support reference types
2 participants