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: Avoid nonce flicker when transaction is submitted #30193

Merged
merged 2 commits into from
Feb 13, 2025
Merged

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Feb 7, 2025

Description

Submitting or cancelling a transaction was clearing the custom nonce field before the confirmation popup had time to close. This PR fixes that.

Open in GitHub Codespaces

Related issues

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

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

cancel.flicker.mov
nonce.goes.to.zero.on.transaction.mov

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 Feb 7, 2025
@pedronfigueiredo pedronfigueiredo self-assigned this Feb 7, 2025
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner February 7, 2025 11:28
Copy link
Contributor

github-actions bot commented Feb 7, 2025

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.

@metamaskbot
Copy link
Collaborator

Builds ready [26c8bf0]
Page Load Metrics (1736 ± 59 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint27119471593448215
domContentLoaded14521921171211957
load14611943173612259
domInteractive25180413416
backgroundConnect989282211
firstReactRender1599432713
getState45919199
initialActions01000
loadScripts10091466124710751
setupStore788232411
uiStartup17042497202717986
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -114 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

@@ -219,8 +217,6 @@ const Footer = () => {
} else {
dispatch(resolvePendingApproval(currentConfirmation.id, undefined));
}
dispatch(updateCustomNonce(''));
dispatch(setNextNonce(''));
Copy link
Contributor

Choose a reason for hiding this comment

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

These calls were added here as it at times resulted in confirmation using old nonce when switching networks. Can you plz check if removing this does not re-introduce that bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing this with @jpuri, she informed me that there used to be a bug in the legacy confirmations that:

  • if you created confirmation on a network
  • edit the nonce
  • and then switch to another network
  • the confirmation page now would show old nonce.

Thus we added this code to clear the old nonce value after confirmation was submitted.

I tested this scenario a few times and it seems to not be an issue in the new confirmations even when removing this code. I will ask our QA engineer to double check this for us.

@metamaskbot
Copy link
Collaborator

Builds ready [b991cd3]
Page Load Metrics (1689 ± 102 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint151524931688210101
domContentLoaded14512425165720498
load151725121689213102
domInteractive20113352211
backgroundConnect879342110
firstReactRender1593382612
getState57013157
initialActions01000
loadScripts10101822120917082
setupStore75620189
uiStartup172828391932258124
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -114 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

@sleepytanya
Copy link
Contributor

No flickering in Chrome and Firefox:

Screen.Recording.2025-02-11.at.11.17.27.mov
Screen.Recording.2025-02-11.at.11.24.47.mov

@pedronfigueiredo pedronfigueiredo added this pull request to the merge queue Feb 13, 2025
Merged via the queue into main with commit 9104b8e Feb 13, 2025
73 checks passed
@pedronfigueiredo pedronfigueiredo deleted the pnf/4140 branch February 13, 2025 10:46
@github-actions github-actions bot locked and limited conversation to collaborators Feb 13, 2025
@metamaskbot metamaskbot added the release-12.13.0 Issue or pull request that will be included in release 12.13.0 label Feb 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed release-12.13.0 Issue or pull request that will be included in release 12.13.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants