feat: Display advanced section within confirmation by default for some users #25687

pedronfigueiredo · 2024-07-04T14:20:47Z

Description

Display advanced details section within confirmation by default for users that have edit nonce or showing hex data on. The PR includes e2e tests for these changes.

This PR also changes the copy for the nonce toggle and includes minor refactors to the gas fees logic.

Related issues

Fixes: #2737

Manual testing steps

Enable nonce editing in the settings
Deploy an erc721 contract in the test dApp
Trigger an erc721 mint in the test dApp
The confirmation should come up with the advanced details toggled on

Screenshots/Recordings

Before

After

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-07-04T14:21:01Z

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.

ui/pages/confirmations/components/confirm/info/contexts/advanced-details-context.tsx

ui/pages/confirmations/components/confirm/header/header-info.test.tsx

ui/pages/confirmations/components/confirm/header/header.test.tsx

ui/pages/confirmations/components/confirm/info/contexts/advanced-details-context.tsx

jpuri · 2024-07-09T12:37:34Z

app/scripts/controllers/preferences.js

@@ -94,6 +94,7 @@ export default class PreferencesController {
        featureNotificationsEnabled: false,
        showTokenAutodetectModal: null,
        showNftAutodetectModal: null, // null because we want to show the modal only the first time
+        isConfirmationAdvancedDetailsOpen: false,


isConfirmationAdvancedDetailsOpen sounds more like app state then preference

It is an app state though, we substituted useState with this background preference property for expanding and collapsing advanced details...

Even though it's triggered via a toggle, it is still a mechanism to indicate their preference which we could theoretically use elsewhere.

So maybe showConfirmationAdvancedDetails?

jpuri · 2024-07-09T16:09:08Z

ui/pages/confirmations/components/confirm/info/contexts/advanced-details-context.tsx

+  const advancedDetailsObject = useMemo(
+    () => ({ showAdvancedDetails, setShowAdvancedDetails }),
+    [showAdvancedDetails, setShowAdvancedDetails],
+  );


useMemo here does not looks very useful.

The useMemo is primarily used for maintaining referential equality and potentially preventing unnecessary re-renders in child components.

I think the container for the properties typically shouldn't matter, as any dependent component or hook would be using the values within the object as dependencies, rather than the container itself.

jpuri · 2024-07-09T16:10:00Z

ui/pages/confirmations/components/confirm/info/contexts/advanced-details-context.test.tsx

+      require('../../../../../../store/actions').setConfirmationAdvancedDetailsOpen;
+
+    getConfirmationAdvancedDetailsOpenMock.mockReturnValue(false);
+    setConfirmationAdvancedDetailsOpenMock.mockReturnValue(null);


spyOn may also be used here for mocking.

jpuri · 2024-07-09T16:10:51Z

ui/components/app/confirm/info/row/text.tsx

+          size={ButtonIconSize.Sm}
+          // to reset the button padding
+          style={{ marginLeft: '-4px' }}
+          data-testid="edit-nonce-icon"


I hope this left margin does not effect existing use cases.

I made the change in the name of consistency with the designs, so I think it's safe.

jpuri · 2024-07-09T16:11:02Z

test/e2e/tests/confirmations/transactions/contract-interaction-redesign.spec.js

+    '.settings-page__header__title-container__close-button',
+  );
+}
+


jpuri

Great work 👍
I left some small feedbacks

…nonce editing (EIP1559)

… without custom nonce editing (EIP1559

sonarqubecloud · 2024-07-17T09:44: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

codecov · 2024-07-17T10:21:39Z

Codecov Report

Attention: Patch coverage is 71.42857% with 14 lines in your changes missing coverage. Please review.

Project coverage is 69.75%. Comparing base (ad5bd9f) to head (5cf58d4).

Files	Patch %	Lines
...firm/info/hooks/useKnownMethodDataInTransaction.ts	0.00%	9 Missing ⚠️
ui/store/actions.ts	0.00%	2 Missing ⚠️
ui/helpers/constants/settings.js	0.00%	1 Missing ⚠️
...rmations/components/confirm/header/header-info.tsx	80.00%	1 Missing ⚠️
.../info/shared/gas-fees-section/gas-fees-section.tsx	66.67%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #25687      +/-   ##
===========================================
- Coverage    69.76%   69.75%   -0.01%     
===========================================
  Files         1398     1400       +2     
  Lines        49171    49199      +28     
  Branches     13574    13578       +4     
===========================================
+ Hits         34303    34318      +15     
- Misses       14868    14881      +13

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

metamaskbot · 2024-07-17T10:34:07Z

Builds ready [5cf58d4]

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, 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 (65 ± 7 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	63	123	94	15	7
		domContentLoaded	9	50	24	13	6
		load	40	99	65	15	7
		domInteractive	9	50	24	13	6

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

background: 886 Bytes (0.03%)
ui: 402 Bytes (0.01%)
common: 121 Bytes (0.00%)

pedronfigueiredo added confirmation-redesign team-confirmations Push issues to confirmations team labels Jul 4, 2024

pedronfigueiredo self-assigned this Jul 4, 2024

pedronfigueiredo requested review from a team as code owners July 4, 2024 14:20

pedronfigueiredo force-pushed the pnf/2737 branch 7 times, most recently from bf3b4da to 90685dd Compare July 5, 2024 10:43

matthewwalsh0 reviewed Jul 5, 2024

View reviewed changes

ui/pages/confirmations/components/confirm/info/contexts/advanced-details-context.tsx Outdated Show resolved Hide resolved

OGPoyraz reviewed Jul 5, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2737 branch 2 times, most recently from fe4cbff to 05e03d0 Compare July 9, 2024 09:23

pedronfigueiredo requested a review from OGPoyraz July 9, 2024 09:30

pedronfigueiredo force-pushed the pnf/2737 branch from 05e03d0 to 9e97025 Compare July 9, 2024 10:13

jpuri reviewed Jul 9, 2024

View reviewed changes

jpuri previously approved these changes Jul 9, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2737 branch 3 times, most recently from a405255 to 314e4aa Compare July 10, 2024 15:37

pedronfigueiredo dismissed jpuri’s stale review via 69a8226 July 10, 2024 16:01

pedronfigueiredo force-pushed the pnf/2737 branch from 314e4aa to 69a8226 Compare July 10, 2024 16:01

matthewwalsh0 approved these changes Jul 17, 2024

View reviewed changes

pedronfigueiredo force-pushed the pnf/2737 branch from 16dc553 to bd64934 Compare July 17, 2024 09:36

pedronfigueiredo and others added 19 commits July 17, 2024 10:41

wip

26673ee

wip

c4a309d

wip

8a31ed9

wip

5fc89bf

wip

1dbee14

wip

856d54e

wip

ff27ca9

wip

7f38aa7

wip

fe8eb70

wip

27a49fe

wip

1afd306

wip

c0877bf

wip

d402fe7

fix test Sends a contract interaction type 2 transaction with custom …

211bc4d

…nonce editing (EIP1559)

remove extra steps on Sends a contract interaction type 2 transaction…

bb9b9de

… without custom nonce editing (EIP1559

remove more extra steps from tests

fa03502

fix lint

a053f40

wip

c1d2f94

wip

5cf58d4

pedronfigueiredo force-pushed the pnf/2737 branch from bd64934 to 5cf58d4 Compare July 17, 2024 09:41

jpuri approved these changes Jul 17, 2024

View reviewed changes

pedronfigueiredo merged commit 8c519db into develop Jul 17, 2024
78 of 79 checks passed

pedronfigueiredo deleted the pnf/2737 branch July 17, 2024 10:22

github-actions bot locked and limited conversation to collaborators Jul 17, 2024

metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Display advanced section within confirmation by default for some users #25687

feat: Display advanced section within confirmation by default for some users #25687

pedronfigueiredo commented Jul 4, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024

jpuri Jul 9, 2024

pedronfigueiredo Jul 10, 2024

matthewwalsh0 Jul 10, 2024

jpuri Jul 9, 2024

pedronfigueiredo Jul 10, 2024

matthewwalsh0 Jul 10, 2024 •

edited

Loading

jpuri Jul 9, 2024

jpuri Jul 9, 2024

pedronfigueiredo Jul 10, 2024

jpuri Jul 9, 2024

jpuri left a comment

sonarqubecloud bot commented Jul 17, 2024

codecov bot commented Jul 17, 2024

metamaskbot commented Jul 17, 2024

feat: Display advanced section within confirmation by default for some users #25687

feat: Display advanced section within confirmation by default for some users #25687

Conversation

pedronfigueiredo commented Jul 4, 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 Jul 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewwalsh0 Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpuri left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jul 17, 2024

Quality Gate passed

codecov bot commented Jul 17, 2024

Codecov Report

metamaskbot commented Jul 17, 2024

pedronfigueiredo commented Jul 4, 2024 •

edited

Loading

matthewwalsh0 Jul 10, 2024 •

edited

Loading