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 TypedDataUtils.hashStruct tests #170

Merged
merged 1 commit into from
Aug 16, 2021
Merged

Add TypedDataUtils.hashStruct tests #170

merged 1 commit into from
Aug 16, 2021

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Aug 11, 2021

This function should now be comprehensively tested. The tests mirror the TypedDataUtils.encodeData tests exactly. Any uses of
hashStruct in the older signature tests have been removed, as they are now redundant.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 11, 2021

This depends upon #169

@Gudahtt Gudahtt force-pushed the add-hash-struct-tests branch 2 times, most recently from cb4c5f5 to e419505 Compare August 11, 2021 22:46
@Gudahtt Gudahtt force-pushed the add-find-type-dependencies-tests branch from f8bb67c to 66ea1b2 Compare August 11, 2021 23:40
@Gudahtt Gudahtt changed the base branch from add-find-type-dependencies-tests to main August 11, 2021 23:43
@Gudahtt Gudahtt marked this pull request as ready for review August 11, 2021 23:43
@Gudahtt Gudahtt requested a review from a team as a code owner August 11, 2021 23:43
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

These tests do mirror the encodeData tests exactly, and I'm wondering why we're going through all the trouble. Given that the only operation of hashStruct is to call ethUtil.keccak with the return value of encodeData. That being the case, what exactly are we testing, here?

Unless we are trying to test keccak itself, it seems to me that this entire PR could be reduced to a single test case:

describe('hashStruct', () => {
  it('hashes the result from encodeData', () => {
    // set up some mocks
    expect(encodeData).toHaveBeenCalledOnce();
    expect(ethUtil.keccak).toHaveBeenCalledOnce();
  })
})

The test should probably be instrumented such that we're convinced that keccak is indeed called with the return value of encodeData, but you get the idea.

@rekmarks rekmarks self-requested a review August 15, 2021 20:40
@rekmarks rekmarks dismissed their stale review August 15, 2021 20:40

I want to discuss the matter, not actually block on it.

@rekmarks
Copy link
Member

I dismissed my previous review because I really meant it as a comment. I want to discuss the matter, but I won't ultimately block on it.

@Gudahtt
Copy link
Member Author

Gudahtt commented Aug 16, 2021

If we were to test just that keccak and encodeData were called, we'd be testing the internals of the function, not the behaviour of the function. If we swapped our keccak implementation out for another one that behaved slightly differently, or if we added operations before or after that call, or if the structure of the encodeData output changed, we'd miss all of those problems. Tests written like that fundamentally aren't useful - they don't do their job, and they make it more difficult to refactor.

The thoroughness here might not be useful though. I did consider testing just a few representative samples. I went with the whole test suite because it seemed just as easy, and I don't know how unlikely it might be for a broken keccack implementation to behave oddly for some Buffer inputs but not others.

This function should now be comprehensively tested. The tests mirror
the `TypedDataUtils.encodeData` tests exactly. Any uses of
`hashStruct` in the older signature tests have been removed, as they
are now redundant.
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit b07aa5d into main Aug 16, 2021
@Gudahtt Gudahtt deleted the add-hash-struct-tests branch August 16, 2021 19:11
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.

2 participants