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: Fix confirmation tx totals on layer 2 networks and for erc20 transfers #23511

Merged
merged 2 commits into from Mar 15, 2024

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Mar 15, 2024

Description

While testing #23510, I discovered that total fees had some inaccuracies in two cases:

  1. On OP layer 2 networks, the rendered total on the confirmation screen was not including the estimated layer 1 fees (as it does in the 'Estimated Fees' section)
  2. For confirmations of the ERC20 transfers, the rendered total that is supposed to be ERC20 token + estimated fee was actually ERC20 token + maximum fee.

The fix for 1 was to bring in fetching of layer1 multi-network fee data into the confirmation-transaction-base component.

The fix for 2 is just to make sure that when we are rendering the transaction detail line where useMax is false, we choose the correct token fee to render (see the new primaryFee variable introduced in this PR).

These two fixes are included in separate commits

Before: Layer 2 total fees

Screenshot from 2024-03-14 22-43-28

After: Layer 2 total fees

Screenshot from 2024-03-14 23-46-02

Before: ERC20 sends

Screenshot from 2024-03-14 23-59-29

After: ERC20 sends

Screenshot from 2024-03-15 00-15-03

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • 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.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

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.

@danjm danjm requested a review from a team as a code owner March 15, 2024 02:53
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.

@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Mar 15, 2024
@danjm danjm changed the title Fix confirm tx totals fix: Fix confirmation tx totals on layer 2 networks and for erc20 transfers Mar 15, 2024
@danjm danjm added the team-confirmations-secure-ux-PR PRs from the confirmations team label Mar 15, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [dda2952]
Page Load Metrics (1314 ± 443 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint681891213316
domContentLoaded968372110
load5623661314923443
domInteractive968372110
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 468 Bytes (0.01%)
  • common: 0 Bytes (0.00%)

@danjm danjm merged commit 5f148e9 into develop Mar 15, 2024
71 of 75 checks passed
@danjm danjm deleted the fix-confirm-tx-totals branch March 15, 2024 10:26
@github-actions github-actions bot locked and limited conversation to collaborators Mar 15, 2024
@metamaskbot metamaskbot added release-11.14.0 Issue or pull request that will be included in release 11.14.0 release-11.12.2 Issue or pull request that will be included in release 11.12.2 and removed release-11.14.0 Issue or pull request that will be included in release 11.14.0 labels Mar 15, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-11.12.2 on PR. Adding release label release-11.12.2 on PR and removing other release labels(release-11.14.0), as PR was cherry-picked in branch 11.12.2.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-11.12.2 Issue or pull request that will be included in release 11.12.2 team-confirmations-secure-ux-PR PRs from the confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants