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

feat: account upgrade confirmation #30347

Open
wants to merge 4 commits into
base: feat/atomic-batch-transactions
Choose a base branch
from

Conversation

matthewwalsh0
Copy link
Member

@matthewwalsh0 matthewwalsh0 commented Feb 15, 2025

Description

Update the transaction confirmation to handle account upgrades via EIP-7702.

Specifically:

  • Add TransactionAccountDetails component.
  • Show new UpgradeCancelModal on Cancel to provide alternate cancel buttons.
  • Add Acknowledge component with required checkbox in order to Confirm transaction.
  • Add accountUpgradeDisabledChains to PreferencesController to persist if user rejects upgrade.
  • Check preference in getCapabilities and processSendCalls middleware hooks.

Open in GitHub Codespaces

Related issues

Fixes: #4214

Manual testing steps

Screenshots/Recordings

Before

After

Confirmation Cancel Modal

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.

@matthewwalsh0 matthewwalsh0 changed the base branch from main to feat/atomic-batch-transactions February 15, 2025 01:45
@matthewwalsh0 matthewwalsh0 added the team-confirmations Push issues to confirmations team label Feb 15, 2025
Emmydon049

This comment was marked as spam.

Emmydon049

This comment was marked as spam.

@matthewwalsh0 matthewwalsh0 added the DO-NOT-MERGE Pull requests that should not be merged label Feb 18, 2025
@matthewwalsh0 matthewwalsh0 force-pushed the feat/atomic-batch-transactions branch from f377c94 to 29af3c0 Compare February 18, 2025 23:59
@metamaskbot
Copy link
Collaborator

Builds ready [3991b93]
Page Load Metrics (1602 ± 44 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint36018261548289139
domContentLoaded1438181015799144
load1449182416029244
domInteractive256833126
backgroundConnect106523157
firstReactRender1469332311
getState45813157
initialActions01000
loadScripts1057139711898440
setupStore7491194
uiStartup17032571186918187

@metamaskbot
Copy link
Collaborator

Builds ready [e4d53f8]
Page Load Metrics (1662 ± 40 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32718081592302145
domContentLoaded1501179716368641
load1534181016628440
domInteractive26106442512
backgroundConnect105630168
firstReactRender1479342512
getState523842
initialActions01000
loadScripts1118137812308139
setupStore76814178
uiStartup17592384194413866

@metamaskbot
Copy link
Collaborator

Builds ready [5f104ac]
Page Load Metrics (1779 ± 106 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint150722161778219105
domContentLoaded149521961759222107
load150922041779220106
domInteractive287540126
backgroundConnect107519157
firstReactRender1496342512
getState580232411
initialActions01000
loadScripts10951688132518187
setupStore761222010
uiStartup175727782126328158

@metamaskbot
Copy link
Collaborator

Builds ready [f658768]
Page Load Metrics (1685 ± 71 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14781996168615173
domContentLoaded14651922166014570
load14751930168514871
domInteractive23393042
backgroundConnect116328157
firstReactRender1467332210
getState670202210
initialActions01000
loadScripts10621471125012861
setupStore86115157
uiStartup17422214195816780

@matthewwalsh0 matthewwalsh0 marked this pull request as ready for review February 21, 2025 10:46
@matthewwalsh0 matthewwalsh0 requested a review from a team as a code owner February 21, 2025 10:46
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 1029 to 1032
state.accountUpgradeDisabledChains = [
...(existingDisabledChains ?? []),
chainId,
];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
state.accountUpgradeDisabledChains = [
...(existingDisabledChains ?? []),
chainId,
];
if (!existingDisabledChains?.includes(chainId)) {
state.accountUpgradeDisabledChains = [
...(existingDisabledChains ?? []),
chainId,
];
}

Would it make sense to add simple if to prevent duplicates?

@matthewwalsh0 matthewwalsh0 force-pushed the feat/atomic-batch-transactions branch from eb51b9c to e38a988 Compare March 7, 2025 21:43
@matthewwalsh0 matthewwalsh0 requested review from a team as code owners March 7, 2025 21:43
Copy link

socket-security bot commented Mar 7, 2025

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

👍 No dependency changes detected in pull request

Add cancel modal.
Add account card.
Add acknowledge checkbox.
@matthewwalsh0 matthewwalsh0 force-pushed the feat/upgrade-confirmation branch from ca24e1e to a7a7f31 Compare March 8, 2025 00:07
@metamaskbot
Copy link
Collaborator

Builds ready [0a0536d]
Page Load Metrics (2073 ± 92 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint33423271880518249
domContentLoaded17502401202717081
load17632465207319192
domInteractive28141482512
backgroundConnect14144493617
firstReactRender1875452010
getState569242210
initialActions01000
loadScripts13271881155214670
setupStore86821199
uiStartup201328762398239115

@metamaskbot
Copy link
Collaborator

Builds ready [298f1a1]
Page Load Metrics (1955 ± 99 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16302266194920197
domContentLoaded16212169190517685
load16302282195520699
domInteractive257235115
backgroundConnect11176524220
firstReactRender146528168
getState5100302713
initialActions01000
loadScripts12091687145115876
setupStore75716147
uiStartup187227992287293141

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO-NOT-MERGE Pull requests that should not be merged team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants