Skip to content
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

chore: update accounts deps #29912

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

chore: update accounts deps #29912

wants to merge 21 commits into from

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Jan 27, 2025

Description

Use the new Snap keyring version that no longer rely on a SnapController reference. Instead it uses the messaging system.

Open in GitHub Codespaces

Note

This PR is blocking other features for the moment, so not every peer deps (related to the AccountsController) have been updated here. This will be done in a separate PR:

Related issues

Manual testing steps

N/A

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.

@ccharly
Copy link
Contributor Author

ccharly commented Jan 27, 2025

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

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

@ccharly ccharly changed the title chore: /update accounts deps chore: update accounts deps Jan 27, 2025
@ccharly ccharly force-pushed the chore/update-accounts-deps branch from 59ea846 to 6086c50 Compare January 30, 2025 12:09
@ccharly
Copy link
Contributor Author

ccharly commented Jan 30, 2025

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

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

@ccharly ccharly force-pushed the chore/update-accounts-deps branch from 3e9c6fa to a95fda7 Compare January 30, 2025 12:47
@metamaskbot
Copy link
Collaborator

Builds ready [0a9f7e4]
Page Load Metrics (1525 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1392169015327536
domContentLoaded1384168115067536
load1393169115257737
domInteractive2299352010
backgroundConnect86223188
firstReactRender1575392512
getState4549115
initialActions01000
loadScripts970124610886632
setupStore7441084
uiStartup15902195174713062
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 37.21 KiB (0.63%)
  • ui: 4.24 KiB (0.05%)
  • common: 182.99 KiB (2.00%)

@ccharly ccharly self-assigned this Jan 30, 2025
@ccharly ccharly removed their assignment Jan 30, 2025
@metamaskbot
Copy link
Collaborator

Builds ready [43889d2]
Page Load Metrics (1709 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint37619491612404194
domContentLoaded14861944168912761
load14901984170913263
domInteractive17132472914
backgroundConnect85122157
firstReactRender1695432713
getState462212211
initialActions01000
loadScripts10581489123411655
setupStore790182211
uiStartup17362396199216680
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 37.21 KiB (0.63%)
  • ui: 4.24 KiB (0.05%)
  • common: 182.99 KiB (2.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [79f106d]
Page Load Metrics (1814 ± 121 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint151624051822267128
domContentLoaded147723441790249120
load151523521814253121
domInteractive257838167
backgroundConnect95930178
firstReactRender1597523015
getState46813168
initialActions01000
loadScripts105918001322207100
setupStore75915147
uiStartup171428852141351169
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 37.21 KiB (0.63%)
  • ui: 4.24 KiB (0.05%)
  • common: 182.99 KiB (2.00%)

@ccharly ccharly marked this pull request as ready for review January 31, 2025 08:52
@ccharly ccharly requested review from a team as code owners January 31, 2025 08:52
Comment on lines +85 to +87
'SnapKeyring:accountAssetListUpdated',
'SnapKeyring:accountBalancesUpdated',
'SnapKeyring:accountTransactionsUpdated',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are now required by the AccountsController

Comment on lines +121 to +123
'SnapKeyring:accountAssetListUpdated',
'SnapKeyring:accountBalancesUpdated',
'SnapKeyring:accountTransactionsUpdated',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are now required by the AccountsController

@@ -68,7 +67,7 @@ const createControllerMessenger = ({
SnapKeyringBuilderAllowActions,
never
>().getRestricted({
name: 'SnapKeyringBuilder',
name: 'SnapKeyring',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to update this since the AccountsController uses this messenger's name now for the new keyring events.


export type SnapKeyringBuilderMessenger = RestrictedControllerMessenger<
'SnapKeyringBuilder',
'SnapKeyring',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to update this since the AccountsController uses this messenger's name now for the new keyring events.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants