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

fix: Fix create metric fragment #28970

Merged
merged 1 commit into from
Dec 6, 2024
Merged

fix: Fix create metric fragment #28970

merged 1 commit into from
Dec 6, 2024

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Dec 5, 2024

Description

The function createEventFragment of the MetaMetricsController was broken recently in the migration to BaseControllerV2 (#28113). We ended up trying to mutate a piece of Immer state, resulting in an error.

The affected line was updated to use cloneDeep prior to mutating, so that we're no longer attempting to mutate a frozen object.

Open in GitHub Codespaces

Related issues

Fixes #28599

Manual testing steps

I'm not sure exactly how to reproduce the error using a real build. But the problem is easy to demonstrate in the "should update existing fragment state with new fragment props" unit test. The problem is that we didn't catch this before because Lodash doesn't have strict mode enabled, so in unit tests the attempt to mutate a frozen object will silently fail. In a production build, strict mode is enabled by LavaMoat.

You can reproduce the problem by adding "use strict" to the top of the file node_modules/lodash/lodash.js and running the test with the {}, removed. Then you can test that adding back {} as the initial parameter fixes the problem.

Screenshots/Recordings

N/A

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.

@Gudahtt Gudahtt force-pushed the fix-metric-event-fragments branch 2 times, most recently from f8ff8f7 to 5348367 Compare December 5, 2024 18:27
@Gudahtt Gudahtt changed the title fix: Fix create/update metric fragment fix: Fix createmetric fragment Dec 5, 2024
@Gudahtt Gudahtt changed the title fix: Fix createmetric fragment fix: Fix create metric fragment Dec 5, 2024
@Gudahtt Gudahtt force-pushed the fix-metric-event-fragments branch from 5348367 to 4f47a46 Compare December 5, 2024 18:47
@metamaskbot
Copy link
Collaborator

Builds ready [4f47a46]
Page Load Metrics (2036 ± 80 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39324661953398191
domContentLoaded16692444200016479
load17262472203616680
domInteractive278942189
backgroundConnect785372512
firstReactRender158325157
getState913081475225
initialActions01000
loadScripts12281855153714670
setupStore718931
uiStartup195230612359244117
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 17 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@Gudahtt Gudahtt force-pushed the fix-metric-event-fragments branch 3 times, most recently from 52a61d4 to 91db2d4 Compare December 5, 2024 23:04
The function `createEventFragment` of the `MetaMetricsController` was
broken recently in the migration to BaseControllerV2 (#28113). We ended
up trying to mutate a piece of Immer state, resulting in an error.

The affected line was updated to use an empty base object as the merge
target, so that we're no longer attempting to mutate a frozen object.

Fixes ##28599
@Gudahtt Gudahtt force-pushed the fix-metric-event-fragments branch from 91db2d4 to 612e9c5 Compare December 5, 2024 23:08
@Gudahtt Gudahtt marked this pull request as ready for review December 5, 2024 23:08
@Gudahtt Gudahtt enabled auto-merge December 5, 2024 23:33
@metamaskbot
Copy link
Collaborator

Builds ready [612e9c5]
Page Load Metrics (2060 ± 113 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26524371798662318
domContentLoaded160223912036233112
load164324402060235113
domInteractive26109502110
backgroundConnect76324189
firstReactRender1594282211
getState773571606230
initialActions01000
loadScripts12051859157119895
setupStore6231032
uiStartup194329472427298143
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 3 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link
Contributor

@desi desi left a comment

Choose a reason for hiding this comment

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

LGTM

@Gudahtt Gudahtt added this pull request to the merge queue Dec 6, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 6, 2024
@Gudahtt Gudahtt added this pull request to the merge queue Dec 6, 2024
Merged via the queue into main with commit aa2823f Dec 6, 2024
75 checks passed
@Gudahtt Gudahtt deleted the fix-metric-event-fragments branch December 6, 2024 00:47
@github-actions github-actions bot locked and limited conversation to collaborators Dec 6, 2024
@metamaskbot metamaskbot added the release-12.10.0 Issue or pull request that will be included in release 12.10.0 label Dec 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.10.0 Issue or pull request that will be included in release 12.10.0 team-extension-platform team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Sentry] [Bug]: TypeError: Cannot add property chain_id, object is not extensible for every send transaction
4 participants