build: track circular dependency in the codebase #29811

dbrans · 2025-01-20T20:30:42Z

Description

This PR introduces circular dependency detection, tracking, and CODEOWNERS.

Here are the key changes:

Added a new circular dependency detection system:
- New script development/circular-deps.ts that uses madge to detect circular dependencies
- New config file .madgerc for TypeScript-specific settings
- Generated baseline file development/circular-deps.json containing current circular dependencies
Added new yarn scripts in package.json:
- yarn circular-deps:check - Verifies circular dependencies match baseline
- yarn circular-deps:update - Updates baseline with current circular dependencies
Added CI integration:
- Added .github/workflows/test-circular-deps.yml to run circular dependency check
Assigning appropriate CODEOWNERS to circular-deps.jsonc

The PR essentially sets up a process to track new circular dependencies while acknowledging existing ones in the codebase.

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3088

Manual testing steps

Remove (or add) a circular dependency in the code.
Run yarn circular-deps:check => should report that codebase is out of sync with circular-deps.jsonc
Run yarn circular-deps:update => should update circular-deps.jsonc to match the codebase. Inspect the diff.
Run yarn circular-deps:check => should pass.

Screenshots/Recordings

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.

…ection

github-actions · 2025-01-20T20:30:57Z

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

metamaskbot · 2025-01-20T21:59:53Z

Builds ready [171032a]

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, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1662 ± 50 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1534	2026	1665	104	50
		domContentLoaded	1484	1972	1641	99	47
		load	1534	2027	1662	105	50
		domInteractive	24	101	43	18	9
		backgroundConnect	6	53	20	15	7
		firstReactRender	16	102	57	29	14
		getState	4	72	16	21	10
		initialActions	0	1	0	0	0
		loadScripts	1078	1446	1183	78	37
		setupStore	6	16	7	2	1
		uiStartup	1716	2482	2030	236	113

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

.madgerc

package.json

HowardBraham · 2025-01-21T20:16:41Z

.github/workflows/test-lint.yml

+      - name: Check circular dependencies
+        run: yarn circular-deps:check


This isn't a great place to put this step, because test-lint was already the slowest job in this part of the pipeline. I would put this step in a separate job that runs on a separate machine.

@HowardBraham I think it should be added to yarn lint and yarn lint:fix, since this really is just a lint step. Developers already run yarn lint:fix (and often yarn lint) before pushing to CI, so while it might slow the lint job in CI a small amount, it won't cause total CI failures due to developers forgetting to run a yarn lint:fix plus an additional circular-lint:fix step.

I think we can do both: If we make the circular dependency fix part of yarn lint:fix for developers, but run the check as a separate CI job to avoid slowing down test-lint. This gives us convenient local fixes and efficient CI checks.

Thoughts?

I've made these changes:

circular-deps:check now runs as a separate CI job

circular-deps:update now is part of yarn lint:fix, which does not run on CI

@dbrans I think you forgot to make this actually run... main.yml needs to call it

package.json

development/circular-deps.ts

socket-security · 2025-01-28T19:15:23Z

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

Package	New capabilities	Transitives	Size	Publisher
npm/@babel/[email protected]	None	`0`	1.91 MB	nicolo-ribaudo
npm/@dependents/[email protected] 🔁 npm/@dependents/[email protected]	None	`0`	6.45 kB	xhmikosr
npm/@jridgewell/[email protected] 🔁 npm/@jridgewell/[email protected]	None	`0`	113 kB	jridgewell
npm/@ts-graphviz/[email protected]	None	`0`	30.5 kB	kamiazya
npm/@ts-graphviz/[email protected]	None	`0`	284 kB	kamiazya
npm/@ts-graphviz/[email protected]	None	`0`	216 kB	kamiazya
npm/@ts-graphviz/[email protected]	None	`0`	86.6 kB	kamiazya
npm/@types/[email protected]	None	`0`	9.66 kB	types
npm/@vue/[email protected] 🔁 npm/@vue/[email protected]	None	`0`	619 kB	yyx990803
npm/@vue/[email protected] 🔁 npm/@vue/[email protected]	None	`0`	639 kB	yyx990803
npm/@vue/[email protected] 🔁 npm/@vue/[email protected]	eval, filesystem, unsafe	`0`	2.62 MB	yyx990803
npm/@vue/[email protected] 🔁 npm/@vue/[email protected]	None	`0`	47.7 kB	yyx990803
npm/@vue/[email protected] 🔁 npm/@vue/[email protected]	None	`0`	86.5 kB	yyx990803
npm/[email protected] 🔁 npm/[email protected]	None	`0`	11.8 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	21 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	10 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	5.17 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	5.39 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	8.56 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	6.65 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	6.67 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	4.54 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	7.94 kB	xhmikosr
npm/[email protected]	None	`0`	5.65 kB	havunen
npm/[email protected] 🔁 npm/[email protected]	None	`0`	212 kB	evilebottnawi
npm/[email protected] 🔁 npm/[email protected]	Transitive: environment	`+3`	23 MB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	6.66 kB	xhmikosr
npm/[email protected]	None	`0`	33.5 kB	ljharb
npm/[email protected]	filesystem, shell	`+1`	250 kB	pahen
npm/[email protected] 🔁 npm/[email protected], npm/[email protected]	None	`0`	467 kB	antfu
npm/[email protected] 🔁 npm/[email protected]	None	`0`	7.57 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	11.2 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	9.28 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	202 kB	ai
npm/[email protected] 🔁 npm/[email protected]	None	`0`	13.3 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	1.28 MB	jrburke
npm/[email protected] 🔁 npm/[email protected]	None	`0`	6.8 kB	xhmikosr
npm/[email protected]	environment, filesystem	`0`	146 kB	ljharb
npm/[email protected] 🔁 npm/[email protected]	None	`0`	9.12 kB	xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	140 kB	7rulnik
npm/[email protected] 🔁 npm/[email protected]	None	`0`	7.52 kB	mrjoelkemp, pahen, xhmikosr
npm/[email protected] 🔁 npm/[email protected]	None	`0`	77.3 kB	kamiazya

🚮 Removed packages: npm/@lgbot/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

socket-security · 2025-01-28T19:15:25Z

🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎

To accept the risk, merge this PR and you will not be notified again.

Alert	Package	Note	Source	CI
Shell access	npm/[email protected]	Module: child_process Location: Package overview	`package.json` `yarn.lock`	🚫

View full report↗︎

Next steps

What is shell access?

This module accesses the system shell. Accessing the system shell increases the risk of executing arbitrary code.

Packages should avoid accessing the shell which can reduce portability, and make it easier for malicious shell access to be introduced.

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@SocketSecurity ignore npm/[email protected]

metamaskbot · 2025-01-28T20:17:53Z

Builds ready [9d4d779]

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, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1776 ± 132 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1407	2388	1777	270	129
		domContentLoaded	1397	2369	1746	262	126
		load	1405	2388	1776	276	132
		domInteractive	23	176	54	36	17
		backgroundConnect	4	82	27	25	12
		firstReactRender	14	121	44	29	14
		getState	4	89	15	19	9
		initialActions	0	0	0	0	0
		loadScripts	1017	1877	1318	229	110
		setupStore	6	59	14	16	8
		uiStartup	1620	2734	2058	334	160

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

dbrans · 2025-01-28T20:59:33Z

@metamaskbot update-policies

HowardBraham · 2025-01-29T19:15:08Z

circular-deps.jsonc is out of sync 🙂, and I think we need one more thing before we can merge this:

Let's add development/circular-deps.jsonc to the CODEOWNERS, possibly controlled by @MetaMask/extension-security-team, or possibly by a new @MetaMask/extension-circular-deps-team?

.github/CODEOWNERS

davidmurdoch

I think I prefer having the check as part of the lint CI step, but not enough to block merging. Approved! Nice work!

dbrans · 2025-01-30T15:34:38Z

@metamaskbot update-policies

metamaskbot · 2025-01-30T15:47:12Z

Policies updated.
👀 Please review the diff for suspicious new powers.

🧠 Learn how: https://lavamoat.github.io/guides/policy-diff/#what-to-look-for-when-reviewing-a-policy-diff

…itoring

metamaskbot · 2025-01-31T08:26:02Z

Builds ready [7dde616]

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, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- content-script: 0
- offscreen: 0

Page Load Metrics (1620 ± 60 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	276	1981	1548	317	152
		domContentLoaded	1372	1920	1598	117	56
		load	1410	1982	1620	125	60
		domInteractive	24	95	44	24	11
		backgroundConnect	8	76	28	21	10
		firstReactRender	15	74	34	22	10
		getState	4	58	15	17	8
		initialActions	0	1	0	0	0
		loadScripts	988	1400	1169	99	48
		setupStore	8	63	20	20	10
		uiStartup	1584	2222	1823	136	66

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2025-01-31T22:59:58Z

Builds ready [bc1c8a2]

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, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13
- ui: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12
- content-script: 0
- offscreen: 0

Page Load Metrics (1702 ± 80 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	430	1936	1562	375	180
		domContentLoaded	1392	2085	1679	160	77
		load	1400	2122	1702	167	80
		domInteractive	22	172	55	35	17
		backgroundConnect	9	83	31	25	12
		firstReactRender	15	74	40	23	11
		getState	4	70	16	19	9
		initialActions	0	1	0	0	0
		loadScripts	980	1615	1224	142	68
		setupStore	6	55	14	13	6
		uiStartup	1559	2337	1916	190	91

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

dbrans added 4 commits January 20, 2025 15:26

build: add madge configuration for TypeScript circular dependency det…

1106021

…ection

feat: add circular dependency detection script

b727bf3

chore: add baseline circular dependencies

aac45a3

ci: integrate circular dependency checks into CI pipeline

7d6639e

metamaskbot added the team-extension-platform Extension Platform team label Jan 20, 2025

Merge branch 'main' into djb/circular-deps-monitoring

6c2d3ba

metamaskbot added INVALID-PR-TEMPLATE PR's body doesn't match template and removed INVALID-PR-TEMPLATE PR's body doesn't match template labels Jan 20, 2025

dbrans added 3 commits January 20, 2025 16:00

fix lint

15d01ca

add file header comment to circular-deps.json and rename to .jsonc

e9fc06d

strip comments from jsonc

171032a

dbrans marked this pull request as ready for review January 20, 2025 21:29

dbrans requested review from davidmurdoch, HowardBraham and Gudahtt January 20, 2025 21:35

gauthierpetetin assigned dbrans Jan 20, 2025

HowardBraham reviewed Jan 21, 2025

View reviewed changes

dbrans added 5 commits January 28, 2025 13:57

move circular-deps.jsonc under the development folder

0c20e95

rename "fix" to "update" and move RESOLUTION_STEPS to top of file

d2992e0

move test-circular-deps check into its own github workflow

6a76a71

update .madgerc

ecec156

Update madge dependency to latest version

2c594bf

dbrans added 2 commits January 28, 2025 14:41

fix madge imports

89c7d81

yarn add -D @types/madge

9d4d779

rebuilt yarn.lock

9220391

dbrans added 2 commits January 29, 2025 14:44

update circular-deps.jsonc

45cf0fb

add circular-deps.jsonc to security team CODEOWNERS

dfa5746

dbrans requested a review from a team as a code owner January 29, 2025 19:48

dbrans commented Jan 29, 2025

View reviewed changes

.github/CODEOWNERS Outdated Show resolved Hide resolved

add @dbrans to circular-deps.jsonc CODEOWNERS

f0b3de1

HowardBraham previously approved these changes Jan 29, 2025

View reviewed changes

HowardBraham requested a review from davidmurdoch January 30, 2025 01:23

davidmurdoch previously approved these changes Jan 30, 2025

View reviewed changes

Update LavaMoat policies

94e3d78

metamaskbot dismissed stale reviews from davidmurdoch and HowardBraham via 94e3d78 January 30, 2025 15:47

Merge remote-tracking branch 'origin/main' into djb/circular-deps-mon…

7dde616

…itoring

HowardBraham previously approved these changes Jan 31, 2025

View reviewed changes

Merge branch 'main' into djb/circular-deps-monitoring

bc1c8a2

HowardBraham dismissed their stale review via bc1c8a2 January 31, 2025 22:25

HowardBraham approved these changes Jan 31, 2025

View reviewed changes

dbrans enabled auto-merge February 1, 2025 12:21

davidmurdoch approved these changes Feb 3, 2025

View reviewed changes

dbrans added this pull request to the merge queue Feb 3, 2025

Merged via the queue into main with commit c89f440 Feb 3, 2025
73 of 74 checks passed

dbrans deleted the djb/circular-deps-monitoring branch February 3, 2025 16:34

github-actions bot locked and limited conversation to collaborators Feb 3, 2025

metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 3, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: track circular dependency in the codebase #29811

build: track circular dependency in the codebase #29811

dbrans commented Jan 20, 2025 •

edited by HowardBraham

Loading

github-actions bot commented Jan 20, 2025

metamaskbot commented Jan 20, 2025

HowardBraham Jan 21, 2025

davidmurdoch Jan 22, 2025

dbrans Jan 28, 2025

dbrans Jan 28, 2025 •

edited

Loading

HowardBraham Jan 29, 2025

dbrans Jan 29, 2025

socket-security bot commented Jan 28, 2025 •

edited

Loading

socket-security bot commented Jan 28, 2025 •

edited

Loading

metamaskbot commented Jan 28, 2025

dbrans commented Jan 28, 2025

HowardBraham commented Jan 29, 2025

davidmurdoch left a comment

dbrans commented Jan 30, 2025

metamaskbot commented Jan 30, 2025

metamaskbot commented Jan 31, 2025

metamaskbot commented Jan 31, 2025

		- name: Check circular dependencies
		run: yarn circular-deps:check

build: track circular dependency in the codebase #29811

build: track circular dependency in the codebase #29811

Conversation

dbrans commented Jan 20, 2025 • edited by HowardBraham Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Jan 20, 2025

metamaskbot commented Jan 20, 2025

HowardBraham Jan 21, 2025

Choose a reason for hiding this comment

davidmurdoch Jan 22, 2025

Choose a reason for hiding this comment

dbrans Jan 28, 2025

Choose a reason for hiding this comment

dbrans Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

HowardBraham Jan 29, 2025

Choose a reason for hiding this comment

dbrans Jan 29, 2025

Choose a reason for hiding this comment

socket-security bot commented Jan 28, 2025 • edited Loading

socket-security bot commented Jan 28, 2025 • edited Loading

Next steps

metamaskbot commented Jan 28, 2025

dbrans commented Jan 28, 2025

HowardBraham commented Jan 29, 2025

davidmurdoch left a comment

Choose a reason for hiding this comment

dbrans commented Jan 30, 2025

metamaskbot commented Jan 30, 2025

metamaskbot commented Jan 31, 2025

metamaskbot commented Jan 31, 2025

dbrans commented Jan 20, 2025 •

edited by HowardBraham

Loading

dbrans Jan 28, 2025 •

edited

Loading

socket-security bot commented Jan 28, 2025 •

edited

Loading

socket-security bot commented Jan 28, 2025 •

edited

Loading