-
-
Notifications
You must be signed in to change notification settings - Fork 54
Update @ethereumjs/util, @ethereumjs/tx, @metamask/eth-sig-util #146
Conversation
Socket Security Pull Request Report👍 No new dependency issues detected in pull request Pull request report summary
Bot CommandsTo ignore an alert, reply with a comment starting with Ignoring: Powered by socket.dev |
@SocketSecurity ignore rlp@2.2.7 @ethereumjs/rlp@4.0.0 |
package.json
Outdated
"ethereumjs-util": "^7.0.9", | ||
"@ethereumjs/tx": "^4.0.0", | ||
"@ethereumjs/util": "^8.0.0", | ||
"@metamask/eth-sig-util": "^5.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This version is deprecated, could you bump up to the latest: v5.0.2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM to me. Would be good to get @darkwing to take a look, and may @PeterYinusa to test this by packing it, adding it to a local extension build and then signing trezor transactions with it if possible.
@mikesposito sorry bout that but looks like this needs another rebase! |
@adonesky1 np, done! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome @mikesposito !
yarn.lock
Outdated
"randombytes@npm:^2.1.0": | ||
version: 2.1.0 | ||
resolution: "randombytes@npm:2.1.0" | ||
dependencies: | ||
safe-buffer: ^5.1.0 | ||
checksum: d779499376bd4cbb435ef3ab9a957006c8682f343f14089ed5f27764e4645114196e75b7f6abf1cbd84fd247c0cb0651698444df8c9bf30e62120fbbc52269d6 | ||
languageName: node | ||
linkType: hard | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like yarn 3.3.1 release relies on randombytes
... but doesn't implement it or import it itself so appears to rely on it being in our dependency tree elsewhere... can this be right @Gudahtt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Agreed that that doesn't sound right. Even if they did rely on that, (oh I see, nvm, it was added to fix this)randombytes
is still in our dependency tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, it seems to be an undeclared dependency in the version of trezor-connect
that we're using
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikesposito I started to investigate this but got sidetracked. Basically trezor-connect
no longer recommends using sub 9.0.0 versions and have marked other versions as deprecated. I started to attempt the migration up to version 9, but it was a bit trickier than I hoped. We should do that as a pre-requisite to completing this ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually a PR to update to v9 already exists: #133.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And it's merged!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This PR updates the following packages to the latest major versions, in order to optimize the extension bundle size.
Changes
ethereumjs-util@^7
substituted with@ethereumjs/util@^8
@ethereumjs/tx@^3
to@ethereumjs/tx@^4
@metamask/eth-sig-util^4
to@metamask/eth-sig-util@^5
@ethereumjs/common^2
to@ethereumjs/common^3
With the new major version of
@ethereumjs/tx@^4
,tx.common.chainIdBN()
has been changed totx.common.chainId()
, as BN.js has been substituted with BigInt.As a consequence, the return type of
chainId()
is now an instance of BigInt, and needed the following changes:chainIdBN().toNumber()
is nowNumber(chainId())
chainIdBN().toString('hex')
is nowchainId().toString(16)
Fixes #15928