-
-
Notifications
You must be signed in to change notification settings - Fork 228
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
Different signTypedData results between 5.1.0 and 6.0.1 #340
Comments
vvvvvv1vvvvvv
changed the title
Different results between 5.1.0 and 6.0.1
Different signTypedData results between 5.1.0 and 6.0.1
Oct 16, 2023
This was referenced Oct 16, 2023
@vvvvvv1vvvvvv Thank you for reporting the issue and your patience. A fix is in #354, which will make your test-case yield the same result as in v5.1.0. Pending releases: |
Gudahtt
added a commit
to MetaMask/metamask-mobile
that referenced
this issue
Nov 20, 2023
In v7.10.0, signatures including a `bytes` field with the value `0x` were being encoded differently than before. Previously the string `0x` was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures. This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when `@metamask/keyring-controller` was updated to v6 (#7267). The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but the `KeyringController` is signing these messages directly rather than delegating to the keyrings, which is why the newer `@metamask/eth-sig-util` package is used here. This has been resovled by patching `@metamask/keyring-controller` to delegate signing typed messages to the keyrings. The very next release of that package includes that change anyway. We will wait until this bug is fixed before updating `@metamask/eth-sig-util` in the keyrings. Fixes #7792
Gudahtt
added a commit
to MetaMask/metamask-mobile
that referenced
this issue
Nov 20, 2023
In v7.10.0, signatures including a `bytes` field with the value `0x` were being encoded differently than before. Previously the string `0x` was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures. This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when `@metamask/keyring-controller` was updated to v6 (#7267). The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but the `KeyringController` is signing these messages directly rather than delegating to the keyrings, which is why the newer `@metamask/eth-sig-util` package is used here. This has been resolved by patching `@metamask/keyring-controller` to delegate signing typed messages to the keyrings. The very next release of that package includes that change anyway. We will wait until this bug is fixed before updating `@metamask/eth-sig-util` in the keyrings. Fixes #7792
13 tasks
This was referenced Nov 21, 2023
Gudahtt
added a commit
to MetaMask/metamask-mobile
that referenced
this issue
Nov 21, 2023
In v7.10.0, signatures including a `bytes` field with the value `0x` were being encoded differently than before. Previously the string `0x` was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures. This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when `@metamask/keyring-controller` was updated to v6 (#7267). The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but the `KeyringController` is signing these messages directly rather than delegating to the keyrings, which is why the newer `@metamask/eth-sig-util` package is used here. This has been resolved by patching `@metamask/keyring-controller` to delegate signing typed messages to the keyrings. The very next release of that package includes that change anyway. We will wait until this bug is fixed before updating `@metamask/eth-sig-util` in the keyrings. Fixes #7792
Gudahtt
added a commit
to MetaMask/metamask-mobile
that referenced
this issue
Nov 29, 2023
## **Description** In v7.10.0, signatures including a `bytes` field with the value `0x` were being encoded differently than before. Previously the string `0x` was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures. This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when `@metamask/keyring-controller` was updated to v6 (#7267). The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but the `KeyringController` is signing these messages directly rather than delegating to the keyrings, which is why the newer `@metamask/eth-sig-util` package is used here. This has been resolved by updating all affected versions of `@metamask/eth-sig-util`. ## **Related issues** Fixes #7792 ## **Manual testing steps** 1. Navigate to https://gudahtt.github.io/test-dapp/ in the in-app browser 2. Sign a message using `eth_signTypedData_v4` 3. Attempt to verify that message. On production (v7.10.0) this does not work. It should work correctly on this branch, returning the signing account address. ## **Screenshots/Recordings** **Before** https://recordit.co/xejXpo4mlh **After** https://recordit.co/NnnpxbCVRB ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
danroc
added a commit
to MetaMask/snap-simple-keyring
that referenced
this issue
Dec 12, 2023
A regression was introduced on eth-sig-util 6.0.1 and 7.0.0, see: MetaMask/eth-sig-util#340
danroc
added a commit
to MetaMask/snap-simple-keyring
that referenced
this issue
Dec 12, 2023
A regression was introduced on eth-sig-util 6.0.1 and 7.0.0, see: MetaMask/eth-sig-util#340
sethkfman
pushed a commit
to MetaMask/metamask-mobile
that referenced
this issue
Dec 22, 2023
## **Description** In v7.10.0, signatures including a `bytes` field with the value `0x` were being encoded differently than before. Previously the string `0x` was interpreted as an "empty" hex number, but as of v7.10.0 they were encoded as the string "0x". This change was not intentional, and it resulted in invalid signatures. This problem was introduced in `@metamask/eth-sig-util@6.0.1` (see here for details: MetaMask/eth-sig-util#340). This package was introduced to mobile when `@metamask/keyring-controller` was updated to v6 (#7267). The keyrings themselves still use `@metamask/eth-sig-util@5.0.1`, but the `KeyringController` is signing these messages directly rather than delegating to the keyrings, which is why the newer `@metamask/eth-sig-util` package is used here. This has been resolved by updating all affected versions of `@metamask/eth-sig-util`. ## **Related issues** Fixes #7792 ## **Manual testing steps** 1. Navigate to https://gudahtt.github.io/test-dapp/ in the in-app browser 2. Sign a message using `eth_signTypedData_v4` 3. Attempt to verify that message. On production (v7.10.0) this does not work. It should work correctly on this branch, returning the signing account address. ## **Screenshots/Recordings** **Before** https://recordit.co/xejXpo4mlh **After** https://recordit.co/NnnpxbCVRB ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [x] In case it's not yet "ready for review", I've set it to "draft". - [x] In case it's "ready for review", I've changed it from "draft" to "non-draft". ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --------- Co-authored-by: legobt <6wbvkn0j@anonaddy.me>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Typed Message
Private Key(empty test address)
7d0ebc17b57137adc429b3f51bf5803fa61e3cd1050e9520d05ed5bcafbcff18
Code example
Result
in v5.1.0, result is
0xb0e34cee18e7ded5fda0cc248f540e003b676a6a24336121df89ead770357334072f6fb61ceb4d840755a497fa8623e0a7404342f7eee7d439bbdaa5c86fbd321c
in versions later than v6.0.1, result is
0xc197219c9b3b41df23f8d52cb5afe2208437f07527c29fbd49af6665f0ef752c1c17c85f758e625bf09e973bf6122e9794687d7e35e03b6ffff04bed973c3b5b1b
The text was updated successfully, but these errors were encountered: