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.

Sign up for GitHub

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

Jump to bottom

fix: legacy gas fee edit modal #7035

Merged
merged 23 commits into from
Sep 22, 2023
Merged

fix: legacy gas fee edit modal #7035

merged 23 commits into from
Sep 22, 2023

Conversation

blackdevelopa
Copy link
Contributor

@blackdevelopa blackdevelopa commented Aug 22, 2023 •
edited

Description
Currently, all legacy transactions on the mobile client uses the 1559 edit gas fee modal when editing gas prices. This PR replaces the legacy transaction edit gas fee modal with its own modal achieving parity with the extension.

Figma

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

Screenshots/Recordings

Issue

fixes #5760

Bitrise build: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/6af15cc0-3753-4daf-bc8d-f1fc6809bd2e

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023 •
edited

Codecov Report

Patch coverage: 50.66% and project coverage change: +0.08% 🎉

Comparison is base (9cf645e) 32.75% compared to head (df3724a) 32.83%.
Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #7035      +/-   ##
==========================================
+ Coverage   32.75%   32.83%   +0.08%     
==========================================
  Files        1006     1006              
  Lines       26957    32718    +5761     
  Branches     2128     8394    +6266     
==========================================
+ Hits         8829    10743    +1914     
- Misses      17701    21975    +4274     
+ Partials      427        0     -427
Files Changed Coverage Δ
...pp/components/UI/ApproveTransactionReview/index.js 2.88% <ø> (-1.08%) ⬇️
app/components/UI/EditGasFeeLegacyUpdate/styles.ts 100.00% <ø> (+33.33%) ⬆️
app/components/UI/TransactionEditor/index.js 2.20% <0.00%> (-0.73%) ⬇️
...ionReview/TransactionReviewEIP1559Update/index.tsx 83.58% <ø> (+16.91%) ⬆️
app/components/Views/ApproveView/Approve/index.js 2.91% <0.00%> (-0.61%) ⬇️
app/components/Views/SendFlow/Confirm/index.js 43.14% <ø> (-0.81%) ⬇️
...actionReview/TransactionReviewInformation/index.js 36.48% <50.00%> (-1.02%) ⬇️
app/components/UI/EditGasFeeLegacyUpdate/index.tsx 61.22% <59.57%> (+8.69%) ⬆️
...pp/components/UI/CustomGasModal/CustomGasModal.tsx 83.63% <100.00%> (-4.12%) ⬇️
app/core/GasPolling/GasPolling.ts 97.95% <100.00%> (+0.73%) ⬆️

... and 662 files with indirect coverage changes

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

@blackdevelopa blackdevelopa changed the title [FIX] Legacy Gas Fee Edit Modal fix: Legacy Gas Fee Edit Modal Aug 22, 2023
@blackdevelopa blackdevelopa added the team-confirmations-secure-ux-PR PR from the confirmations team label Aug 22, 2023
@blackdevelopa blackdevelopa marked this pull request as ready for review August 25, 2023 02:51
@blackdevelopa blackdevelopa requested a review from a team as a code owner August 25, 2023 02:51
@blackdevelopa blackdevelopa self-assigned this Aug 25, 2023
@blackdevelopa blackdevelopa added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Aug 25, 2023
@blackdevelopa blackdevelopa changed the title fix: Legacy Gas Fee Edit Modal fix: legacy gas fee edit modal Aug 25, 2023
@jpuri
Copy link
Contributor

jpuri commented Aug 30, 2023

Changes look largely good, I left some small feedbacks.

digiwand
digiwand reviewed Aug 30, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

looking like a good fix. I wonder if we should plan to add more test coverage

digiwand
digiwand previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

code LGTM! I'd suggest having this QA'd as I'm not seeing the labels for this yet

@blackdevelopa
Copy link
Contributor Author

code LGTM! I'd suggest having this QA'd as I'm not seeing the labels for this yet

Yes we will. I intend to move it to QA after getting dev approval.

@seaona
Copy link
Contributor

seaona commented Sep 1, 2023 •
edited

I'll add here the QA findings:
[Edit] Oh I realized there's 1 dev approval missing. I'll continue QA once we have it 🙏

Send ERC20 Token from Dapp displays legacy fees

Whenever I am on an EIP1559 network, if I trigger the Send ERC20 token function I can see how the gas displayed is Legacy instead of EIP1559.

send-token-legacy-fees.mp4

Repro:

  1. Select Sepolia opr Goerli network
  2. Go to the test dapp
  3. Connect Wallet
  4. Click Deploy ERC20
  5. Click Transfer ERC20
  6. Check gas displays legacy modal

jpuri
jpuri previously approved these changes Sep 1, 2023
View reviewed changes
@blackdevelopa blackdevelopa added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Sep 1, 2023
@seaona seaona added QA'd - Issues Found QA has been complete, however issues have been discovered that need to be addressed and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Sep 4, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

55.2% 55.2% Coverage
0.0% 0.0% Duplication

jpuri
jpuri approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@jpuri jpuri left a comment

Choose a reason for hiding this comment

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

LGTM

digiwand
digiwand approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

re: #7035 (comment)

as proposed in ^ this comment, it seems others have agreed this PR is okay to merge based on a conversation @blackdevelopa showed me. It has been discussed by others that the issues mentioned above can be addressed in follow-up PRs

@blackdevelopa blackdevelopa merged commit 97c861e into main Sep 22, 2023
25 checks passed
@blackdevelopa blackdevelopa deleted the 5760/legacy-txn-gas-modal branch September 22, 2023 08:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jpuri jpuri jpuri approved these changes

@Gudahtt Gudahtt Gudahtt left review comments

@digiwand digiwand digiwand approved these changes

Assignees

@blackdevelopa blackdevelopa

Labels
release-7.9.0 team-confirmations-secure-ux-PR PR from the confirmations team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug] Gas Fee modal for Legacy tx is displaying the EIP1559 fee modal
7 participants
@blackdevelopa @codecov-commenter @jpuri @seaona @Gudahtt @digiwand @metamaskbot