Skip to content

test: test withKeyring for readonly ops #32414

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

ccharly
Copy link
Contributor

@ccharly ccharly commented Apr 30, 2025

Description

Testing:

Open in GitHub Codespaces

Related issues

N/A

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.

Copy link

socket-security bot commented Apr 30, 2025

No dependency changes detected. Learn more about Socket for GitHub.

👍 No dependency changes detected in pull request

@ccharly ccharly force-pushed the test/with-keyring branch from 597464e to 951ed90 Compare April 30, 2025 14:18
@ccharly ccharly force-pushed the test/with-keyring branch from 951ed90 to 48f2a59 Compare May 5, 2025 18:10
@metamaskbot
Copy link
Collaborator

Builds ready [48f2a59]
UI Startup Metrics (1244 ± 71 ms)
PlatformBuildTypePageMetricMean (ms)Min (ms)Max (ms)Std Dev (ms)P 75 (ms)P 95 (ms)
ChromeBrowserifyHomeuiStartup1244109514317112991369
load107891212477211331183
domContentLoaded107190612417211281174
domInteractive18146571729
firstPaint780107124841711171180
backgroundConnect84375820
firstReactRender21154972142
getState1364671826
initialActions001001
loadScripts83067798168890927
setupStore85233914
WebpackHomeuiStartup21721692254319023162436
load16891274206515417851905
domContentLoaded16811267196115017771890
domInteractive161066111348
firstPaint1736761577203290
backgroundConnect349384433669
firstReactRender17454362108295344
getState144157161633
initialActions316135
loadScripts16761265193615017751872
setupStore197181182432
FirefoxBrowserifyHomeuiStartup13511175179911714001612
load12091031165112012601495
domContentLoaded12091031165112012601495
domInteractive1023728535116156
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect18134851927
firstReactRender22185552230
getState7430389
initialActions001001
loadScripts11931018163712012441481
setupStore64385611
WebpackHomeuiStartup15431366202913916031881
load13281177179212813831619
domContentLoaded13271176179112813831619
domInteractive79391461887112
firstPaintNaNNaNNaNNaNNaNNaN
backgroundConnect22145072340
firstReactRender35295653744
getState104377930
initialActions101011
loadScripts13081162176912713661604
setupStore95748827
Benchmark value 1245 exceeds gate value 1234 for chrome browserify home mean uiStartup
Benchmark value 1079 exceeds gate value 1070 for chrome browserify home mean load
Benchmark value 1072 exceeds gate value 1061 for chrome browserify home mean domContentLoaded
Benchmark value 831 exceeds gate value 830 for chrome browserify home mean loadScripts
Benchmark value 1369 exceeds gate value 1365 for chrome browserify home p95 uiStartup
Benchmark value 20 exceeds gate value 18 for chrome browserify home p95 backgroundConnect
Benchmark value 1481 exceeds gate value 1475 for firefox browserify home p95 loadScripts
Sum of mean exceeds: 32ms | Sum of p95 exceeds: 12ms
Sum of all benchmark exceeds: 44ms

ccharly added a commit to MetaMask/core that referenced this pull request May 5, 2025
…withKeyring` (#5732)

## Explanation

We were always calling `#updateVault` when using `withKeyring` even when
the keyring was not mutated.

This PR now compare the states after the operation execution and see if
the keyring got updated or not and decide to call `#updateVault` only if
needed.

Testing PR:
- MetaMask/metamask-extension#32414

## References

N/A

## Changelog

N/A

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've communicated my changes to consumers by [updating changelogs
for packages I've
changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs),
highlighting breaking changes as necessary
- [ ] I've prepared draft pull requests for clients and consumer
packages to resolve any breaking changes

---------

Co-authored-by: Michele Esposito <[email protected]>
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.

2 participants