chore(deps): bump `@metamask/eth-trezor-keyring` to `^6.0.0` by mikesposito · Pull Request #27689 · MetaMask/metamask-extension

mikesposito · 2024-10-08T08:19:17Z

Description

This PR bumps @metamask/eth-trezor-keyring from ^3.1.3 to ^6.0.0.

## [6.0.0]

### Added

- **BREAKING:** Add ESM build ([#40](https://github.com/MetaMask/accounts/pull/40))
  - It's no longer possible to import files from `./dist` directly.

## [5.0.0]

### Changed

- **BREAKING**: Bump `@metamask/eth-sig-util` dependency from `^7.0.3` to `^8.0.0` ([#79](https://github.com/MetaMask/accounts/pull/79))
  - `signTypedData` no longer support `number` for addresses, see [here](https://github.com/MetaMask/eth-sig-util/blob/main/CHANGELOG.md#800).

## [4.0.0]

### Changed

- **BREAKING**: `addAccounts` will now only return newly created accounts ([#64](https://github.com/MetaMask/accounts/pull/64))
  - This keyring was initially returning every accounts (previous and new ones), which is different from what is expected in the [`Keyring` interface].(https://github.com/MetaMask/utils/blob/v9.2.1/src/keyring.ts#L65)

Related issues

Fixes:

Manual testing steps

These changes directly impact Trezor devices:

Add one or more Trezor accounts
Sign message
Sign typed data
Sign transaction
Remove Trezor accounts

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

socket-security · 2024-10-08T08:20:06Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/eth-trezor-keyring@6.0.0	None	`0`	160 kB	metamaskbot
npm/@metamask/slip44@4.0.0	None	`0`	136 kB	metamaskbot

🚮 Removed packages: npm/@metamask/eth-trezor-keyring@3.1.3, npm/@metamask/slip44@4.1.0, npm/tslib@2.6.2

View full report↗︎

sonarqubecloud · 2024-10-08T13:33:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mikesposito · 2024-12-10T09:42:29Z

package.json

    "cross-spawn@npm:^5.0.1": "^7.0.6",
-    "@solana/web3.js@npm:^1.95.0": "^1.95.8"
+    "@solana/web3.js@npm:^1.95.0": "^1.95.8",
+    "tslib": "~2.6.0"


this tslib forced resolution has been added to fix this error that prevents the wallet unlock:

TypeError#1: Cannot assign to read only property 'Symbol(Symbol.iterator)' of object '[object Object]' (anonymous) @ sentry-install.js:37776 sentry-install.js:37776 at Object.__generator (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-8.js:49554:131) at Mutex.<anonymous> (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-5.js:22679:28) at chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-8.js:49548:41 at new Promise (<anonymous>) at Object.__awaiter (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-8.js:49544:16) at Mutex.acquire (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-5.js:22676:24) at CurrencyRateController.updateExchangeRate (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-1.js:21928:42) at CurrencyRateController._executePoll (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-1.js:21994:16) at chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/common-1.js:24712:24 at sentryWrapped (chrome-extension://bclblcklhpbhilglihnehjhmpfchmmpp/scripts/sentry-install.js:21642:17)

Two suggestions: Could we drop tslib as a top-level dependency, and can we narrow this resolution further?

The resolution is currently applied to all copies of tslib in the dependency tree, not just the one that was causing an issue here. Some of the affected versions are asking for a range that doesn't include 2.6 (e.g. there was on 1.x copy)

The direct dependency has been added because it is a peer dependency of @trezor/connect-web, which is a dependency of both the keyring and the client (because of the offscreen client-defined keyring). I suppose that some other package already provided it, but I thought it would be good to add it explicitly. Do you think we should remove it?

Regarding the resolution, it is indeed too broad, and we are getting rid of 1.x - I'll try to narrow it down

On a sidenote, I believe we have wrongly added a "direct" dep to tslib on Trezor keyring here:

https://github.com/MetaMask/accounts/blob/main/packages/keyring-eth-trezor/package.json#L56

We probably should have use a peer dep for this one instead... I'll check/track this internally to avoid having this resolution (cause I do believe we could get rid of it if we the dependency was done correctly on our side)

I think that this issue is due to a specific version range of tslib which is incompatible with certain parts of the extension, so we probably would have experienced it regardless at some point

metamaskbot · 2024-12-10T10:15:47Z

Builds ready [458e0a1]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1992 ± 88 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	319	2437	1815	505	242
		domContentLoaded	1705	2362	1959	179	86
		load	1713	2382	1992	184	88
		domInteractive	25	70	38	13	6
		backgroundConnect	7	105	33	26	13
		firstReactRender	17	91	31	23	11
		getState	98	304	172	73	35
		initialActions	0	1	0	0	0
		loadScripts	1329	1929	1532	156	75
		setupStore	7	83	17	22	10
		uiStartup	1940	2940	2409	272	131

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 56.62 KiB (1.08%)
ui: 0 Bytes (0.00%)
common: -906 Bytes (-0.01%)

mikesposito · 2025-01-08T11:01:30Z

@metamaskbot update-policies

metamaskbot · 2025-01-08T11:14:07Z

No policy changes

metamaskbot · 2025-01-08T12:24:46Z

Builds ready [06a0b65]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1593 ± 83 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1381	2049	1596	175	84
		domContentLoaded	1375	2031	1572	168	80
		load	1388	2051	1593	173	83
		domInteractive	22	131	46	30	14
		backgroundConnect	8	63	28	20	9
		firstReactRender	15	75	30	22	11
		getState	4	62	18	19	9
		initialActions	0	1	0	0	0
		loadScripts	997	1556	1166	138	66
		setupStore	6	71	13	18	9
		uiStartup	1524	2332	1826	212	102

ccharly

LGTM, I rely on e2e tests for validation though.

metamaskbot · 2025-01-08T14:53:04Z

Builds ready [37aed22]

builds: chrome, firefox
builds (flask): chrome, firefox
builds (MMI): chrome
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Bundle Size Stats
mv2: E2e Actions Stats
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6, 7
- common: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 13, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1614 ± 132 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1383	2610	1609	259	124
		domContentLoaded	1340	2566	1586	251	121
		load	1385	2681	1614	275	132
		domInteractive	21	66	30	11	5
		backgroundConnect	8	120	28	30	14
		firstReactRender	15	72	39	24	12
		getState	4	41	9	8	4
		initialActions	0	0	0	0	0
		loadScripts	963	1924	1169	196	94
		setupStore	6	51	11	13	6
		uiStartup	1577	3038	1812	305	146

Akaryatrh · 2025-01-09T11:07:12Z

Good to me. Tested locally and was able to test successfully (with Trezor simulator) the provided scenario:

Add one or more Trezor accounts
Sign message
Sign typed data
Sign transaction
Remove Trezor accounts

chore(deps): bump trezor keyring to ^4.0.0

21e2b61

github-actions bot added the team-wallet-framework label Oct 8, 2024

mikesposito added 3 commits October 8, 2024 10:26

chore: yarn dedupe

cb30fb2

chore: update policies

9031033

Merge branch 'develop' into mikesposito/update-trezor

7f59a9a

mikesposito added 2 commits November 13, 2024 15:29

Merge branch 'develop' into mikesposito/update-trezor

5d24414

Merge branch 'develop' into mikesposito/update-trezor

90bb095

mikesposito added team-wallet-framework and removed team-wallet-framework labels Nov 19, 2024

mikesposito marked this pull request as ready for review November 19, 2024 12:32

mikesposito requested review from a team as code owners November 19, 2024 12:32

mikesposito self-assigned this Nov 19, 2024

Merge branch 'main' into mikesposito/update-trezor

92bd47b

mikesposito changed the title ~~chore(deps): bump trezor keyring to ^4.0.0~~ chore(deps): bump trezor keyring to ^6.0.0 Dec 9, 2024

mikesposito added 3 commits December 10, 2024 10:39

chore: add tslib to deps and force resolution

5ed7418

chore: update policies

71ccffd

Merge branch 'main' into mikesposito/update-trezor

458e0a1

mikesposito commented Dec 10, 2024

View reviewed changes

mikesposito changed the title ~~chore(deps): bump trezor keyring to ^6.0.0~~ chore(deps): bump @metamask/eth-trezor-keyring to ^6.0.0 Dec 10, 2024

gantunesr previously approved these changes Dec 10, 2024

View reviewed changes

mikesposito added 2 commits January 8, 2025 11:05

Merge branch 'main' into mikesposito/update-trezor

aa4e47e

update policies

1fc0772

mikesposito dismissed gantunesr’s stale review via 1fc0772 January 8, 2025 10:13

refactor: narrow down tslib resolution

71d22a4

mikesposito requested review from Gudahtt and gantunesr January 8, 2025 10:38

refactor: broad resolution for other versions

e57851a

add missing resolutions

06a0b65

ccharly approved these changes Jan 8, 2025

View reviewed changes

Merge branch 'main' into mikesposito/update-trezor

37aed22

mikesposito enabled auto-merge January 8, 2025 15:18

Akaryatrh approved these changes Jan 9, 2025

View reviewed changes

mikesposito added this pull request to the merge queue Jan 9, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 9, 2025

mikesposito added this pull request to the merge queue Jan 9, 2025

Merged via the queue into main with commit 00a5db7 Jan 9, 2025

mikesposito deleted the mikesposito/update-trezor branch January 9, 2025 12:42

github-actions bot locked and limited conversation to collaborators Jan 9, 2025

metamaskbot added the release-12.11.0 Issue or pull request that will be included in release 12.11.0 label Jan 9, 2025

Conversation

mikesposito commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

socket-security bot commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 8, 2024

Quality Gate passed

Uh oh!

mikesposito Dec 10, 2024

Choose a reason for hiding this comment

Uh oh!

Gudahtt Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

mikesposito Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

ccharly Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

mikesposito Jan 8, 2025

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Dec 10, 2024

Uh oh!

mikesposito commented Jan 8, 2025

Uh oh!

metamaskbot commented Jan 8, 2025

Uh oh!

metamaskbot commented Jan 8, 2025

Uh oh!

ccharly left a comment

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Jan 8, 2025

Uh oh!

Akaryatrh commented Jan 9, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mikesposito commented Oct 8, 2024 •

edited

Loading

socket-security bot commented Oct 8, 2024 •

edited

Loading

mikesposito Dec 11, 2024 •

edited

Loading