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

upgrade eth-sig-util #1039

Merged
merged 23 commits into from
Nov 28, 2022
Merged

upgrade eth-sig-util #1039

merged 23 commits into from
Nov 28, 2022

Conversation

goosewobbler
Copy link
Contributor

@goosewobbler goosewobbler commented Sep 13, 2022

  • removed eth-sig-util and upgraded all usage to @metamask/eth-sig-util
  • improved typed data typing
  • version enums everywhere

TODO:

  • manual test hot signers

mholtzman
mholtzman previously approved these changes Sep 16, 2022
main/signers/lattice/Lattice/index.ts Outdated Show resolved Hide resolved
@mholtzman mholtzman added the WIP PRs that are still in progress and not ready for review or merging label Sep 26, 2022
@goosewobbler goosewobbler added maintenance hardware signers hot signers and removed WIP PRs that are still in progress and not ready for review or merging labels Nov 16, 2022
Copy link
Collaborator

@mholtzman mholtzman left a comment

Choose a reason for hiding this comment

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

this looks good to me, just a couple comments. also do we want to merge this into the deps branch?

main/provider/index.ts Outdated Show resolved Hide resolved
@@ -1,5 +1,5 @@
const crypto = require('crypto')
const ethSigUtil = require('eth-sig-util')
const { signTypedData } = require('@metamask/eth-sig-util')
Copy link
Collaborator

Choose a reason for hiding this comment

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

the worker has access to sensitive data so we just need to make sure every time we change or add a dependency here. this looks OK to me but just tagging @floating for double review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, ideally we would use ethers for this too if we can. As discussed last week I think we get this in and then update again to switch to ethers if possible.

main/signers/hot/HotSigner/worker.js Outdated Show resolved Hide resolved
@goosewobbler goosewobbler changed the base branch from develop to deps November 27, 2022 15:54
@socket-security
Copy link

socket-security bot commented Nov 27, 2022

Socket Security Pull Request Report

👍 No new dependency issues detected in pull request

Pull request report summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

Powered by socket.dev

main/provider/index.ts Outdated Show resolved Hide resolved
@goosewobbler goosewobbler merged commit 94f3a76 into deps Nov 28, 2022
@goosewobbler goosewobbler deleted the upgrade-eth-sig-util branch November 28, 2022 14:30
goosewobbler added a commit that referenced this pull request Nov 29, 2022
Co-authored-by: goosewobbler <goosewobbler@pm.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants