Skip to content

refactor: Add MultichainRouter to new controller instantiation pattern #31951

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

jiexi
Copy link
Contributor

@jiexi jiexi commented Apr 14, 2025

Description

Cleans up the MultichainRouter instantiation by moving it int othe new controller instantiation pattern.

Open in GitHub Codespaces

Related issues

See: https://github.com/orgs/MetaMask/projects/146/views/6?pane=issue&itemId=105350449&issue=MetaMask%7CMetaMask-planning%7C4599

Manual testing steps

No user facing changes

Screenshots/Recordings

Before

After

Pre-merge author checklist

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.

Copy link
Contributor

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.

messenger: controllerMessenger,
// Binding the call to provide the selector only giving the controller the option to pass the operation
withSnapKeyring: (...args) =>
// @ts-expect-error mistmatch with the withSnapKeyring signature and withKeyring.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue tracking this mismatch we can link?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't think we need an issue tracking this specifically. Frederik is aware that this hook needs to be called from the messenger inside the MultichainRouter implementation which will allow us to remove it here and resolves the type issue

@metamaskbot
Copy link
Collaborator

Builds ready [bed7cd6]
UI Startup Metrics (1191 ± 62 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1191107414076212201314
load103294412135410611159
domContentLoaded102694112055310541150
domInteractive17136391629
firstPaint701130121340110291103
backgroundConnect74162710
firstReactRender21164462134
getState1356591728
initialActions001001
loadScripts78771495151818916
setupStore75182713
WebpackHomeuiStartup21181729255016022072343
load16221266199412917091796
domContentLoaded16141261198312917031792
domInteractive15114471339
firstPaint1847548069203334
backgroundConnect289356362658
firstReactRender22155390123345369
getState134194201427
initialActions612792835
loadScripts16091258195912817001791
setupStore1764481934
FirefoxBrowserifyHomeuiStartup13451166166510214101558
load12011047152510512551417
domContentLoaded12011047152510512551417
domInteractive1023923932112152
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2013121132038
firstReactRender22193232328
getState73202710
initialActions001001
loadScripts11821023150910612411400
setupStore6435468
WebpackHomeuiStartup14971332197012915731787
load12811136171011913361521
domContentLoaded12811136171011913361521
domInteractive80432392790136
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22147382341
firstReactRender35295653744
getState105598830
initialActions102111
loadScripts12621120169211913201494
setupStore95416825
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.46 KiB (0.03%)
  • ui: 2 Bytes (0%)
  • common: -1 Bytes (0%)

@github-project-automation github-project-automation bot moved this to Needs dev review in PR review queue Apr 15, 2025
Copy link
Contributor

@adonesky1 adonesky1 left a comment

Choose a reason for hiding this comment

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

LGTM

@metamaskbot
Copy link
Collaborator

Builds ready [3aa4834]
UI Startup Metrics (1221 ± 67 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1221109914076712711341
load105594712666310991168
domContentLoaded105094412606310941163
domInteractive18136371729
firstPaint73870126841410621163
backgroundConnect74223812
firstReactRender21146362234
getState1454592033
initialActions001001
loadScripts810709100361843924
setupStore85182812
WebpackHomeuiStartup21741801270616022682439
load16881391204112017841854
domContentLoaded16811387203112017751850
domInteractive161156101448
firstPaint1877841865228319
backgroundConnect3210366493164
firstReactRender20658386121330362
getState154182191535
initialActions318146
loadScripts16771385200712017721848
setupStore236293392243
FirefoxBrowserifyHomeuiStartup13771204173111214411585
load12261039156411212991429
domContentLoaded12251039156311212991429
domInteractive1043723531119172
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect2513199232274
firstReactRender24195252433
getState84576811
initialActions002001
loadScripts12031024146911012851402
setupStore74456622
WebpackHomeuiStartup15651365205014816201943
load13381181179313213831680
domContentLoaded13381181179213213831679
domInteractive86372573094130
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect23146692346
firstReactRender37305653947
getState94627929
initialActions104111
loadScripts13181167177513113651644
setupStore95304921
cc: @HowardBraham
Benchmark value 33 exceeds gate value 31 for chrome browserify home p95 getState
Benchmark value 187 exceeds gate value 175 for chrome webpack home mean firstPaint
Benchmark value 319 exceeds gate value 310 for chrome webpack home p95 firstPaint
Benchmark value 74 exceeds gate value 70 for firefox browserify home p95 backgroundConnect
Benchmark value 1943 exceeds gate value 1935 for firefox webpack home p95 uiStartup
Benchmark value 1680 exceeds gate value 1660 for firefox webpack home p95 load
Benchmark value 1679 exceeds gate value 1660 for firefox webpack home p95 domContentLoaded
Benchmark value 46 exceeds gate value 45 for firefox webpack home p95 backgroundConnect
Benchmark value 1644 exceeds gate value 1630 for firefox webpack home p95 loadScripts
Sum of mean exceeds: 12ms | Sum of p95 exceeds: 77ms
Sum of all benchmark exceeds: 89ms

Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.46 KiB (0.03%)
  • ui: 5 Bytes (0%)
  • common: 11 Bytes (0%)

@jiexi jiexi added the no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed label Apr 17, 2025
const controller = new MultichainRouter({
messenger: controllerMessenger,
// Binding the call to provide the selector only giving the controller the option to pass the operation
withSnapKeyring: (...args) =>
Copy link
Member

Choose a reason for hiding this comment

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

We will need to rethink this as we had to make a change to this function to unblock the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog no-changelog Indicates no external facing user changes, therefore no changelog documentation needed team-wallet-api-platform
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

4 participants