chore: bump @ethereumjs/tx to v5 - #321
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning MetaMask internal reviewing guidelines:
|
33fab99 to
d1ea1a7
Compare
d1ea1a7 to
0a2afb5
Compare
a050878 to
d6b02aa
Compare
d6b02aa to
6f38d4e
Compare
6f38d4e to
e72ff48
Compare
The merge-base changed after approval.
e72ff48 to
580b594
Compare
580b594 to
35af8e3
Compare
35af8e3 to
a39b3bc
Compare
a39b3bc to
76cdf67
Compare
Pull Request is not mergeable
76cdf67 to
1de0a93
Compare
@ethereumjs/tx ^4.2.0 -> ^5.4.0
The bump compiles with no source change, which is exactly the trap here.
@ethereumjs/tx@5 reuses the name TxData for something entirely different:
v4 TxData = a legacy transaction data object
v5 TxData = { [TransactionType.Legacy]: LegacyTxData, [AccessListEIP2930]: ..., ... }
v4's meaning is now called LegacyTxData. Since keyring.ts declares
signTransaction as returning Promise<TxData>, the bump alone would silently
change that public type from "a signed legacy transaction" to "an object
carrying every transaction type at once", and still build clean.
Confirmed by typechecking a v4 era consumer against the unpatched build:
Type '{ nonce, gasPrice, gasLimit, to, value, data, v, r, s }' is missing
the following properties from type 'TxData': [TransactionType.Legacy],
[TransactionType.AccessListEIP2930], ...
Mapping TxData to LegacyTxData restores the original contract exactly, so
the same consumer typechecks again and the bump is not breaking. TypedTxData
would also accept it, but it is a union, so callers would have to narrow the
result. LegacyTxData keeps the API identical to v4.
1de0a93 to
7d78cba
Compare
Top of stack #315, on #321. `yarn test:types` runs bare `tsd`, which finds **no test files at all**. It produces no output and exits 0 regardless, so the 460 lines of type assertions across `src/*.test-d.ts` have not been checking anything. Proof, before this change: ``` $ printf 'expectAssignable<Hex>(999);' >> src/hex.test-d.ts $ yarn test:types $ echo $? 0 ``` Pointing tsd at the files explicitly reports it properly and exits 1: ``` ✖ Argument of type 999 is not assignable to parameter of type `0x${string}`. ``` ## Cause tsd resolves test files relative to the `types` field, which has pointed at `./dist/index.d.cts` since ts-bridge was adopted in #182 (2024-04-23). tsd 0.29 does not resolve `.d.cts`, and the `tsd.directory` setting does not compensate. **26 releases** have shipped since, all with this check silently passing. ## Fix ```diff - "test:types": "tsd" + "test:types": "tsd --files 'src/*.test-d.ts'" ``` Fixing the invocation rather than the `types` field, since the field is correct for consumers and only tsd is confused by it. All four files pass once actually executed, so none of them needed changing. `yarn test:types` now exits 1 on a bad assertion and 0 when clean. ## Relevance to the migration This was found while working out what to do with tsd in Phase B, since core has no way to run it. Worth knowing the honest baseline before deciding: these assertions have been dormant for 17 months, so whatever we do with them in core, we are not losing coverage we currently have. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Only the npm script for type testing changed; runtime package behavior and public types are unchanged. > > **Overview** > **`yarn test:types` was a no-op:** bare `tsd` never picked up the `src/*.test-d.ts` suites (because of how it resolves files vs the package `types` entry), so type assertions could fail silently while CI still exited 0. > > The script now runs **`tsd --files 'src/*.test-d.ts'`**, so those assertions actually execute and **`yarn test:types` fails when a type check is wrong**. No changes to the test files themselves were required once they were being run. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit b934063. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY -->
Top of stack #315, on #320. Last of the PR#2 bumps.
@ethereumjs/tx^4.2.0^5.4.0This is not breaking, but only because of a one word change
The bump compiles with no source change at all, which is the trap.
@ethereumjs/tx@5reuses the nameTxDatafor something entirely different:v4's meaning is now called
LegacyTxData.keyring.tsdeclaressignTransactionas returningPromise<TxData>, so bumping alone would silently change that public type from "a signed legacy transaction" into "an object carrying every transaction type at once", and still build clean.Typechecking a v4 era consumer against the unpatched build confirms it:
Mapping
TxDatatoLegacyTxDatarestores the original contract exactly, and that same consumer typechecks again.TypedTxDatawould also accept it, but it is a union, so callers would have to narrow the result.LegacyTxDatakeeps the API identical to v4.Note
Keyringis already deprecated in favour of@metamask/keyring-utils, so the blast radius is small either way.Note
Medium Risk
Touches transaction typing on a deprecated but public Keyring API; the explicit LegacyTxData fix avoids a silent type break, but downstream packages must align with @ethereumjs/tx v5.
Overview
Upgrades
@ethereumjs/txfrom^4.2.0to^5.4.0(with transitive bumps to@ethereumjs/common,util,rlp, and related crypto packages in the lockfile).Because v5 redefines
TxDataas a per–transaction-type map instead of a single legacy field bag, the deprecatedKeyring.signTransactionreturn type is updated fromPromise<TxData>toPromise<LegacyTxData>, preserving the same runtime shape and v4-era TypeScript contract for consumers. No implementation changes are required for keyring authors.Reviewed by Cursor Bugbot for commit 7d78cba. Bugbot is set up for automated code reviews on this repo. Configure here.