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(23946): Add advanced details component #24833

Merged
merged 34 commits into from
Jun 21, 2024
Merged

feat(23946): Add advanced details component #24833

merged 34 commits into from
Jun 21, 2024

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented May 28, 2024

Description

Adds an advanced details subsection. This component is conditionally rendered when the button is selected in the header.

It includes nonce information that may be edited if the setting has been toggled. It also includes hex data and the function signature from 4bytes (if this 3rd party has been enabled).

Open in GitHub Codespaces

Related issues

Fixes: #23946

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

After

Screenshot 2024-05-28 at 17 11 23

Pre-merge author checklist

  • I’ve followed MetaMask 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.

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/23946 branch 7 times, most recently from 7fcec54 to 3c0c7ab Compare May 29, 2024 14:59
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Looking great! Stories make it very easy for me to review for UI it makes a huge difference. Made a couple small suggestions

@sleepytanya
Copy link
Contributor

Not sure how it is expected to work:

  • edit nonce is available when 'Customize transaction nonce' toggle is OFF (Advanced Settings)
  • nonce appears to be editable, new nonce value is shown on the confirmation screen but the transaction is sent with the default nonce
  • scroll button appears only if the message is scrolled all the way to the bottom and then up again

https://jam.dev/c/4c831549-d1da-4638-935c-d64d0de6e6c7

Screen.Recording.2024-05-29.at.13.04.13.mov

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/23946 branch 2 times, most recently from e07bb4c to 0d3f22e Compare June 3, 2024 12:20
@pedronfigueiredo
Copy link
Contributor Author

pedronfigueiredo commented Jun 3, 2024

@sleepytanya Thanks for reviewing.

edit nonce is available when 'Customize transaction nonce' toggle is OFF (Advanced Settings)

This should be fixed now

nonce appears to be editable, new nonce value is shown on the confirmation screen but the transaction is sent with the default nonce

I believe this behaviour is by default. You cannot submit a tx with a higher nonce than the current one, or a lower nonce if a tx with that nonce has been already approved.

The only way I can think of for testing this, is to submit a transaction with very low gas and immediately send another transaction with the same nonce with higher gas to replace it. However, this cannot currently be tested as the Gas controls are not yet available on this PR. I suggest we take note of this scenario and revisit it once that work is merged.

scroll button appears only if the message is scrolled all the way to the bottom and then up again

I wasn't able to reproduce it and couldn't see the video

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/23946 branch 2 times, most recently from fe1811f to 2d0bd9d Compare June 3, 2024 14:49
@sleepytanya
Copy link
Contributor

sleepytanya commented Jun 4, 2024

@pedronfigueiredo

I wasn't able to reproduce it and couldn't see the video

Sorry, I may have deleted the video accidentally. New link:

https://jam.dev/c/ce1b802b-5620-42d7-a5d7-4c25665b3336

Nonce is non-editable when the nonce customization is off:

https://jam.dev/c/2b5280e8-b231-4360-bdb1-638f31c27adc

Not sure if using higher nonce was addressed yet but I checked just in case (doesn't work):

https://jam.dev/c/7720eaea-b518-4047-bf21-b08c46af2a0b

Looks like custom nonce is being ignored altogether- here the transaction with the low custom nonce is sent with the subsequent nonce value:

https://jam.dev/c/7eb917b8-d6dc-49de-92c1-ab7e659747d9

Expected behavior - attempt to send a tx with the low nonce results in the warning 'nonce is too low' and failed tx (unless user is trying to cancel pending tx with the same nonce).

@sleepytanya
Copy link
Contributor

sleepytanya commented Jun 4, 2024

@pedronfigueiredo

Looks beautiful!

  1. The scroll arrow is functioning great.
  2. When attempting to submit a transaction with a low nonce, a notification indicating 'tx nonce is too low..' is displayed and transaction is not submitted (this notification does not appear during video recording so I omitted this part in the video).
  3. Transactions with a high nonce are submitted with a custom nonce value, and successfully displayed in a 'Pending' state within the Activity tab.

Video: https://jam.dev/c/32aecd57-ddd8-454d-8049-4262d48238b4

@pedronfigueiredo pedronfigueiredo force-pushed the pnf/23946 branch 2 times, most recently from 907f591 to 9cc859b Compare June 5, 2024 10:40
@@ -90,6 +104,27 @@ const HeaderInfo = () => {
}}
/>
</Tooltip>
{isShowAdvancedDetailsToggle && (
Copy link
Contributor

Choose a reason for hiding this comment

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

non-blocking: would be good to have a story that shows this

Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

UI LGTM!

matthewwalsh0
matthewwalsh0 previously approved these changes Jun 13, 2024
@matthewwalsh0 matthewwalsh0 dismissed stale reviews from georgewrmarshall and themself via 7159a89 June 19, 2024 11:40
matthewwalsh0
matthewwalsh0 previously approved these changes Jun 19, 2024
vinistevam
vinistevam previously approved these changes Jun 19, 2024
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

Attention: Patch coverage is 72.22222% with 25 lines in your changes missing coverage. Please review.

Project coverage is 64.94%. Comparing base (523af49) to head (0b51574).
Report is 8 commits behind head on develop.

Files Patch % Lines
.../info/shared/advanced-details/advanced-details.tsx 73.33% 12 Missing ⚠️
...confirmations/components/confirm/footer/footer.tsx 50.00% 7 Missing ⚠️
...rmations/components/confirm/header/header-info.tsx 44.44% 5 Missing ⚠️
...saction-base/confirm-transaction-base.container.js 50.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #24833      +/-   ##
===========================================
+ Coverage    64.93%   64.94%   +0.01%     
===========================================
  Files         1385     1386       +1     
  Lines        54870    54947      +77     
  Branches     14408    14428      +20     
===========================================
+ Hits         35625    35681      +56     
- Misses       19245    19266      +21     

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

@metamaskbot
Copy link
Collaborator

Builds ready [0b51574]
Page Load Metrics (48 ± 4 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint64997894
domContentLoaded9261142
load39694894
domInteractive8261142
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 0 Bytes (0.00%)
  • ui: 8.17 KiB (0.12%)
  • common: 308 Bytes (0.00%)

@matthewwalsh0 matthewwalsh0 merged commit 5ce1822 into develop Jun 21, 2024
74 checks passed
@matthewwalsh0 matthewwalsh0 deleted the pnf/23946 branch June 21, 2024 08:09
@github-actions github-actions bot locked and limited conversation to collaborators Jun 21, 2024
@metamaskbot metamaskbot added the release-12.1.0 Issue or pull request that will be included in release 12.1.0 label Jun 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
confirmation-re-design confirmation-redesign QA Passed release-12.1.0 Issue or pull request that will be included in release 12.1.0 team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Tech Task] Implement info subsection for advanced view in contract interaction
8 participants