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.

Sign up for GitHub

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

Jump to bottom

fix: KeepKey produces accurate EIP-712 eth_signTypedData_v4 signatures #1

Closed
wants to merge 26 commits into from
Closed

fix: KeepKey produces accurate EIP-712 eth_signTypedData_v4 signatures #1

wants to merge 26 commits into from

Conversation

0xMillz
Copy link
Member

@0xMillz 0xMillz commented Oct 18, 2023

Duplicate of this shapeshift/hdwallet PR: shapeshift#649

Officially closes the hdwallet part of issue shapeshift#578, which likely never worked.
Related issue: shapeshift/web#5384

I went with the EthereumSignTypedHash approach for ALL TypedData messages after consulting with @BitHighlander. It seems that the KeepKey firmware is missing the EthereumTypedDataStructRequest type, which would be required for a fully human readable, eip-712-compatible message to be displayed on the device screen.

Fortunately, the EthereumSignTypedHash approach allows the KeepKey to at least sign the v4 messages, which for end-users that means being able to do modern web3 interactions like buying and selling NFTs on OpenSea, for example.

-The bulk of the fix is in packages/hdwallet-keepkey/src/ethereum.ts

  • I also added a recoverTypedSignature check to the sandbox so you know right away if the signature that the KeepKey spits out is accurate.
    -I refactored the Ethereum sandbox tests too. That file is getting way too large and repetitive, but still is the best way to test with KeepKey.
  • I couldn't help but fix some other random some bugs along the way.
  • Feel free to point out anything extra that I should change/remove from this PR--I went in not knowing if I would be able to fix this, so I tried to improve my developer experience at the very least as I went through all the code!
  • If approved, version bumps will be required.

Thanks!

image

*KeepKey Bridge didn't seem to be working for me in the sandbox so I used the regular webusb option instead.

0xMillz and others added 26 commits October 18, 2023 04:09
@0xMillz
Fix ethSignTypedData so that the signing address can be recovered fro…
f847bf5 
…m the signature
@0xMillz
Update Sandbox ethSignTypedData to verify that signature is accurate …
1045c4a 
…by recovering the signing address + refactor the Ethereum Sandbox section + tech debt + random bug fixes caught along the way + messing around with tsconfig
@gomesalexandre
feat: ledger fixes and improvements (shapeshift#644)
38eeeb7
@0xMillz
Merge in master, fix conflicts
33d5302
@0xMillz
Merge in master again, fix conflicts
c4766cf
@0xApotheosis
perf: optimize seed creation (shapeshift#648)
6805444
@gomesalexandre
feat: ledger thorchain support (shapeshift#643)
e8a7539
@gomesalexandre
fix: make keplr great again (shapeshift#646)
c0dddd4
@BitHighlander
Merge branch 'master' into fix_kk_ethSignTypedData
d1ab981
@0xMillz
Revert "Merge branch 'master' into fix_kk_ethSignTypedData"
b5fa182 
This reverts commit d1ab981, reversing
changes made to c4766cf.

Reverts accidental blind merge from master
@0xMillz
remove ethereumjs-util since we already have web3.utils
f51a8bf
@0xMillz
fix: messageHash should not be set if "EIP712Domain" is the primaryType
cd29461
@0xMillz
Add additional test cases
76ae5d1
@0xMillz
bump hdwallet-* versions
dd5b547
@0xMillz
Revert "Revert "Merge branch 'master' into fix_kk_ethSignTypedData""
6b2ce3a 
This reverts commit b5fa182 and resolves all conflicts

Lesson for future: use `git reset` to "revert" a merge commit or bad things can happen
@gomesalexandre @kaladinlight
feat: cashaddr getWalletPublicKeys format (shapeshift#650)
12288a2 
Co-authored-by: kaladinlight <[email protected]>
@kaladinlight
chore: regen yarn lock (shapeshift#652)
11727c2
@gomesalexandre
perf: don't import * as ethers (shapeshift#651)
de3593e
@0xMillz
Remove dependencies browserify-zlib and update-browserslist-db (a Sky…
5f8b3e3 
…net-type nightmare). Remove some tsconfig changes based on PR feedback.
@0xMillz
update yarn.lock by running yarn
3206d6d
@0xMillz
move parcel back to a regular dep
78ed4ae
@0xMillz
update yarn.lock
f6d4e25
@0xMillz
merge in master, fix conflicts
870e307
@0xMillz
bump all to v1.52.11
a45f5a8
@0xMillz
remove update-browserslist-db + caniuse-lite changes from yarn.lock
2503066
@0xMillz
ts wants this for import $ from "jquery"
640d634
@0xMillz
Copy link
Member Author

0xMillz commented Oct 31, 2023

This change is included in PR #3

@0xMillz 0xMillz closed this Oct 31, 2023
@0xMillz 0xMillz deleted the fix_kk_ethSignTypedData branch December 8, 2023 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@0xMillz @gomesalexandre @0xApotheosis @BitHighlander @kaladinlight