-
-
Notifications
You must be signed in to change notification settings - Fork 226
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 TypedDataUtils.eip712Hash
unit tests
#173
Conversation
|
54b00e0
to
5384ba6
Compare
3dd529b
to
fab6ec5
Compare
eip712Hash
unit testsTypedDataUtils.eip712Hash
unit tests
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.
My initial impression here is the same as #170 (review), namely that these tests seem perhaps uselessly redundant. I'm not sure why it wouldn't suffice to check that it makes the expected calls to sanitizeData
, hashStruct
, and ethUtil.keccak
given the possible branches inside the function, and rely on the encodeData
tests for testing a wide range of inputs.
fab6ec5
to
b9402a2
Compare
Unit tests have been added for `TypedDataUtils.eip712Hash`. These tests don't exhaustively check all possible inputs because there are a lot, and they're just passed along to `hashStruct`. So the `hashStruct` tests should catch any input-specific changes. Outside of behaviour internal to `hashStruct`, all of the behaviour should be covered by these tests. Any calls to `eip712Hash` in the older signature tests have been removed, as they are now redundant.
b9402a2
to
030b96b
Compare
This has been rebased to resolve conflicts |
I think these tests are mostly devoid of repeats actually! I only included a few sample inputs here. Most of it was focused on behaviour specific to this function, about how it encodes the type header and the data and concatenates them together, with the data being optional and omitted in some circumstances. And the peculiar range of expected inputs, since it will happily accept inputs that are contrary to the input types. |
66bcbbe
to
42c0e66
Compare
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.
LGTM!
Unit tests have been added for
TypedDataUtils.eip712Hash
. These tests don't exhaustively check all possible inputs because there are a lot, and they're just passed along tohashStruct
. So thehashStruct
tests should catch any input-specific changes. Outside of behaviour internal tohashStruct
, all of the behaviour should be covered by these tests.Any calls to
eip712Hash
in the older signature tests have been removed, as they are now redundant.