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

Js sign typed data comparison testing #241

Merged
merged 12 commits into from Oct 26, 2023

Conversation

pacrob
Copy link
Collaborator

@pacrob pacrob commented Oct 17, 2023

What was wrong?

sign_typed_data has been implemented with the intention of producing similar results as metamask and ethers-js, but the current test cases only compare with a static value (hash or sig)

How was it fixed?

  • Built on the existing javascript mnemonic testing, verify messages encode to the same sig where expected. Test cases explain why where they don't match.
  • Fixed a bug in encoding where the order of encoding the domain data was determined by the data (a json object, which is unordered) instead of the standard domain types (an array, which is ordered).

Todo:

  • Clean up commit history

  • Add or update documentation related to these changes

  • Add entry to the release notes

Cute Animal Picture

image

@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from 9494469 to 5130560 Compare October 17, 2023 22:24
@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from 6f6f72f to 07327a4 Compare October 18, 2023 21:07
@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from b8d4522 to 35fcebc Compare October 24, 2023 18:08
@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch 2 times, most recently from a3e331c to 9021aa9 Compare October 25, 2023 18:25
@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from 9021aa9 to 9de2aa3 Compare October 25, 2023 18:28
@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from dfeebec to 33a1cbe Compare October 25, 2023 19:59
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

Nit over the naming, but overall lgtm!

- run:
name: Build ethers-cli
command: cd tests/integration/ethers-cli && sudo npm install && sudo npm install -g . && cd ../../../
name: Build js-integration-test-scripts
Copy link
Contributor

Choose a reason for hiding this comment

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

This name seems redundant since it is located in the integration directory. I don't have context about the ethers tests or what that name was about. Do the tests encompass tests for any js client? Would be curious if there is a name that aligns with what the tests do?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ethers is to JS as web3py is to python - https://github.com/ethers-io/ethers.js/tree/e454afb2fa3612c273a09b154c36b35fafab17b1

Prior to this PR, the integration tests there only used a specific set of functions from ethers, so the name was appropriate. Since I've added tests that use both ethers and a metamask library, I figured a more general name was in order.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree on the redundancy - I'll clean that up.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okey, shortened to js-scripts

Copy link
Contributor

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

First pass looks great to me 🔥

I had a question on one of the validations we do that matches ethers.js but not metamask. I'm going to run the tests now and make sure I understand how they are set up on the JS side as my JS is rusty 😅 ... but the rest looks good to me 👍🏼

Copy link
Contributor

@fselmo fselmo left a comment

Choose a reason for hiding this comment

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

lgtm 👍🏼

@pacrob pacrob force-pushed the js-sign-typed-data-comparison-testing branch from cd1fef9 to 5f4c7be Compare October 26, 2023 22:09
@pacrob pacrob merged commit 5ea07e9 into ethereum:master Oct 26, 2023
23 checks passed
@pacrob pacrob deleted the js-sign-typed-data-comparison-testing branch October 26, 2023 22:31
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.

None yet

3 participants