fix: allow getAddTransactionRequest to pass through other params #27117

martahj · 2024-09-12T22:01:13Z

Description

Updates getAddTransactionRequest to pass through additional parameters so that waitForSubmit will get passed through to addTransaction .

Previously, the waitForSubmit param, although passed by addTransaction and addTransactionAndWaitForPublish, was not included in the object returned by getAddTransactionRequest. This doesn't seem to have had negative consequences on a standard wallet, but when using a hardware wallet, it meant that addTransactionAndWaitForPublish resolved before waiting for the user to accept or reject the transaction. As a consequence, when doing a swap, the user was taken directly to the "Processing..." screen before they had a chance to take action on the transaction. If they rejected the transaction, they remained on that screen indefinitely.

Related issues

Fixes: https://consensyssoftware.atlassian.net/browse/MMS-1189

Manual testing steps

Disable smart transactions
Confirm that the following flows work and display relevant and expected screens using both a hardware and standard wallet:

Swap that does not need approval step - accept tx
Swap that does not need approval step - reject tx
Swap that needs approval step - reject approval tx
Swap that needs approval step - accept approval tx, reject trade tx
Swap that needs approval step - accept both txs
Send + swap that does not need approval step - accept tx
Send + swap that does not need approval step - reject tx
Send + swap that needs approval step - reject approval tx
Send + swap that needs approval step - accept approval tx, reject trade tx
Send + swap that needs approval step - accept both txs

Screenshots/Recordings

Before

Video from bug report (shows rejecting swap flow):

noSTX.mov

After

Accepting swap:

IMG_8102.MOV

Rejecting swap:

IMG_8103.MOV

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

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.

github-actions · 2024-09-12T22:01:26Z

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.

…waitForSubmit will not get dropped

sonarcloud · 2024-09-13T14:28:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

codecov · 2024-09-13T14:47:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.05%. Comparing base (fb72eca) to head (09bab84).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #27117      +/-   ##
===========================================
+ Coverage    70.04%   70.05%   +0.01%     
===========================================
  Files         1435     1435              
  Lines        49927    49927              
  Branches     13981    13981              
===========================================
+ Hits         34971    34975       +4     
+ Misses       14956    14952       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2024-09-13T14:59:46Z

Builds ready [09bab84]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1972 ± 106 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	228	2349	1872	434	208
		domContentLoaded	1515	2328	1954	229	110
		load	1552	2344	1972	221	106
		domInteractive	14	93	40	22	11

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 10 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

davibroc

I have tested the fix and it works well now @martahj @dan437

dan437

LGTM

app/scripts/metamask-controller.test.js

Co-authored-by: Derek Brans <dbrans@gmail.com>

metamaskbot · 2024-10-07T20:57:27Z

Builds ready [3f1749d]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1798 ± 80 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	1591	2206	1802	161	77
		domContentLoaded	1584	2198	1775	152	73
		load	1592	2208	1798	167	80
		domInteractive	26	164	48	31	15

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 10 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

sonarcloud · 2024-10-08T19:16:21Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-10-08T19:36:27Z

Builds ready [23f116c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5, 6
- common: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1979 ± 99 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	396	2267	1725	522	250
		domContentLoaded	1720	2501	1937	193	93
		load	1728	2507	1979	206	99
		domInteractive	16	254	74	58	28

Bundle size diffs [🚨 Warning! Bundle size has increased!]

background: 10 Bytes (0.00%)
ui: 0 Bytes (0.00%)
common: 0 Bytes (0.00%)

martahj added the team-extension-platform label Sep 12, 2024

martahj force-pushed the swapping-hardware-wallet-bug branch from b896b4b to cd3cd38 Compare September 13, 2024 13:32

martahj added the team-swaps label Sep 13, 2024

fix: allow getAddTransactionRequest to pass through other options so …

09bab84

…waitForSubmit will not get dropped

martahj force-pushed the swapping-hardware-wallet-bug branch from fafd0d8 to 09bab84 Compare September 13, 2024 14:26

martahj marked this pull request as ready for review September 13, 2024 14:59

martahj requested a review from a team as a code owner September 13, 2024 14:59

davibroc previously approved these changes Sep 20, 2024

View reviewed changes

dan437 previously approved these changes Sep 24, 2024

View reviewed changes

dbrans previously approved these changes Oct 7, 2024

View reviewed changes

dbrans reviewed Oct 7, 2024

View reviewed changes

app/scripts/metamask-controller.test.js Outdated Show resolved Hide resolved

martahj dismissed stale reviews from dbrans and dan437 via 455abaa October 7, 2024 18:09

fix: only test for passthrough params in related test

455abaa

Co-authored-by: Derek Brans <dbrans@gmail.com>

martahj dismissed davibroc’s stale review via 455abaa October 7, 2024 18:09

Merge branch 'develop' into swapping-hardware-wallet-bug

3f1749d

dbrans approved these changes Oct 7, 2024

View reviewed changes

infiniteflower approved these changes Oct 8, 2024

View reviewed changes

Merge branch 'develop' into swapping-hardware-wallet-bug

23f116c

martahj added this pull request to the merge queue Oct 8, 2024

Merged via the queue into develop with commit f41a625 Oct 8, 2024
78 checks passed

martahj deleted the swapping-hardware-wallet-bug branch October 8, 2024 19:51

github-actions bot locked and limited conversation to collaborators Oct 8, 2024

metamaskbot added the release-12.7.0 Issue or pull request that will be included in release 12.7.0 label Oct 8, 2024

gauthierpetetin added release-12.6.0 Issue or pull request that will be included in release 12.6.0 and removed release-12.7.0 Issue or pull request that will be included in release 12.7.0 labels Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: allow getAddTransactionRequest to pass through other params #27117

fix: allow getAddTransactionRequest to pass through other params #27117

martahj commented Sep 12, 2024 •

edited

Loading

github-actions bot commented Sep 12, 2024

sonarcloud bot commented Sep 13, 2024

codecov bot commented Sep 13, 2024 •

edited

Loading

metamaskbot commented Sep 13, 2024

davibroc left a comment

dan437 left a comment

metamaskbot commented Oct 7, 2024

sonarcloud bot commented Oct 8, 2024

metamaskbot commented Oct 8, 2024

fix: allow getAddTransactionRequest to pass through other params #27117

fix: allow getAddTransactionRequest to pass through other params #27117

Conversation

martahj commented Sep 12, 2024 • edited Loading

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Sep 12, 2024

sonarcloud bot commented Sep 13, 2024

Quality Gate passed

codecov bot commented Sep 13, 2024 • edited Loading

Codecov Report

metamaskbot commented Sep 13, 2024

davibroc left a comment

Choose a reason for hiding this comment

dan437 left a comment

Choose a reason for hiding this comment

metamaskbot commented Oct 7, 2024

sonarcloud bot commented Oct 8, 2024

Quality Gate passed

metamaskbot commented Oct 8, 2024

martahj commented Sep 12, 2024 •

edited

Loading

codecov bot commented Sep 13, 2024 •

edited

Loading