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: Delete legacy transaction confirmation code #29926

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

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Jan 27, 2025

Description

The goal of this PR is to remove as much dead code as possible related to the transaction confirmations that predate the redesign.

  • From ConfirmTransaction component:

    • ConfirmDeployContract
    • ConfirmSendEther
    • ConfirmContractInteraction
  • From ConfirmTokenTransactionSwitch and ConfirmTransactionSwitch:

    • ConfirmApprove
    • ConfirmTokenTransactionBase
    • ConfirmSendToken
    • ConfirmTransactionBase, TokenAllowance and ConfirmApproveContent that were used by the components above.
  • GasDisplay, ConfirmGasDisplay and ConfirmLegacyGasDisplay that were no longer used

  • Edit custom nonce settings toggle, selector, action and state property migration, and associated custom-nonce component for the legacy transactions.

  • Unit tests, storybook stories, styles and localization files associated with the removed files.

Open in GitHub Codespaces

Related issues

Fixes: https://github.com/MetaMask/MetaMask-planning/issues/4025

Manual testing steps

Confirm the remaining confirmations (Transactions, Signatures, Encrypt / Decrypt, Add/Switch Ethereum Chain Snaps) still work.

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.

@pedronfigueiredo pedronfigueiredo added the team-confirmations Push issues to confirmations team label Jan 27, 2025
@pedronfigueiredo pedronfigueiredo self-assigned this Jan 27, 2025
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner January 27, 2025 16:33
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.

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/4025 branch 3 times, most recently from 64e5531 to b5c9fc8 Compare January 27, 2025 16:56
@metamaskbot
Copy link
Collaborator

Builds ready [b5c9fc8]
Page Load Metrics (1783 ± 105 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint145823201782226108
domContentLoaded144822531750212102
load146122711783218105
domInteractive25118442411
backgroundConnect1187302211
firstReactRender16102442914
getState560192010
initialActions00000
loadScripts10101773129919493
setupStore893172110
uiStartup170128062092292140
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -196.6 KiB (-2.43%)
  • common: -5.92 KiB (-0.06%)

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/4025 branch 2 times, most recently from 8c28873 to 1659a71 Compare January 27, 2025 17:49
@pedronfigueiredo
Copy link
Contributor Author

@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

@metamaskbot metamaskbot requested review from a team as code owners January 27, 2025 18:12
@metamaskbot
Copy link
Collaborator

Builds ready [9d7ada6]
Page Load Metrics (1813 ± 66 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint16182230181913665
domContentLoaded16022219178313464
load16202230181313766
domInteractive2699412110
backgroundConnect1195372613
firstReactRender1796502813
getState588232512
initialActions01000
loadScripts10971593129510852
setupStore86215157
uiStartup181726372114225108
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: -196.69 KiB (-2.43%)
  • common: -5.96 KiB (-0.07%)

@pedronfigueiredo
Copy link
Contributor Author

@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

@pedronfigueiredo
Copy link
Contributor Author

@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

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/4025 branch 2 times, most recently from 93f769e to 86c9928 Compare January 28, 2025 17:07
@pedronfigueiredo
Copy link
Contributor Author

@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

@metamaskbot
Copy link
Collaborator

Builds ready [ffa5475]
Page Load Metrics (1616 ± 63 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint40918571553292140
domContentLoaded14361849159312158
load14461865161613163
domInteractive239233199
backgroundConnect88623199
firstReactRender1598462914
getState475192110
initialActions01000
loadScripts10121386115010651
setupStore861222110
uiStartup166225321887226108
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 444 Bytes (0.01%)
  • ui: -197.79 KiB (-2.44%)
  • common: -6.61 KiB (-0.07%)

@metamaskbot
Copy link
Collaborator

Builds ready [e12f346]
Page Load Metrics (1534 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1426167215377737
domContentLoaded1398162115096833
load1427166815347536
domInteractive157031157
backgroundConnect884262211
firstReactRender1492352512
getState44811126
initialActions00000
loadScripts993120810976732
setupStore76415168
uiStartup15772152175613062
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 444 Bytes (0.01%)
  • ui: -207.89 KiB (-2.57%)
  • common: -6.61 KiB (-0.07%)

@pedronfigueiredo
Copy link
Contributor Author

@MetaMask/policy-reviewers, I believe the policy change results from removing the package @storybook/addon-knobs.

I don't understand why browserify is now added to the policy file.

-  "@storybook/addon-knobs>qs": {
+  "browserify>url>qs": {

davidmurdoch
davidmurdoch previously approved these changes Jan 30, 2025
Copy link
Contributor

@davidmurdoch davidmurdoch left a comment

Choose a reason for hiding this comment

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

Policy changes are approved. No new permissions added. Only the path to the qs module has changed due to package updates/removals.

@sleepytanya
Copy link
Contributor

sleepytanya commented Jan 31, 2025

@pedronfigueiredo
I don't see 'Customize tx nonce' in the Settings->Advanced (I'm using the dev build):

Screen.Recording.2025-01-31.at.00.32.16.mov

@sleepytanya
Copy link
Contributor

sleepytanya commented Jan 31, 2025

@pedronfigueiredo
Looks like I can't Cancel or Speed up the transaction:
Update: likely not related to this PR as the same issue is present in RC 12.12.0 #30027

Screen.Recording.2025-01-31.at.00.39.30.mov

@naugtur
Copy link
Contributor

naugtur commented Jan 31, 2025

@pedronfigueiredo

 -  "@storybook/addon-knobs>qs": {
 +  "browserify>url>qs": {

The change is in the identifier of the qs package. After the dependency tree change, the shortest path in the graph leading to this specific copy of qs is this. It's probably because addon-knobs updated to a major version that was deduplicated to the top before (something else depended on it too) or it got rid of qs altogether.

I'll include a complete explanation of this in the next educational content :) There's a hint of it in the policy review process training currently available in cornerstone.

@pedronfigueiredo
Copy link
Contributor Author

@pedronfigueiredo I don't see 'Customize tx nonce' in the Settings->Advanced (I'm using the dev build):

@sleepytanya this has been removed on this PR as it was only needed in the old confirmation flows. In the redesigned screens the nonce is always editable as long as advanced details are open and smart transactions are turned off.

@Bdiit
Copy link

Bdiit commented Jan 31, 2025

Nicer 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-confirmations Push issues to confirmations team
Projects
Status: Needs dev review
Development

Successfully merging this pull request may close these issues.

6 participants