-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
Conversation
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
There was a problem hiding this 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.
src/sign-typed-data.test.ts
Outdated
describe('TYPED_MESSAGE_SCHEMA', () => { | ||
it('should match valid typed message', () => { | ||
const ajv = new Ajv(); | ||
const validate = ajv.compile(TYPED_MESSAGE_SCHEMA); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
src/sign-typed-data.test.ts
Outdated
}); | ||
|
||
for (const solidityType of eip712SolidityTypes) { | ||
// eslint-disable-next-line no-loop-func |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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