From 5478b5c29433c55fe9803ed189ad4c85e5993f38 Mon Sep 17 00:00:00 2001 From: Olusegun Akintayo Date: Thu, 29 Feb 2024 16:21:22 +0300 Subject: [PATCH 1/4] fix: Transaction Confirmation incorrectly showing max instead of estimated fees and total (#23203) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** The Transaction Confirmation screen is incorrectly showing max fees and totals where estimated values should be: We should show the estimated and max fee and total independently: [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/23203?quickstart=1) ## **Related issues** Fixes: [#2112](https://github.com/MetaMask/MetaMask-planning/issues/2112) ## **Manual testing steps** 1. Launch MM 2. Perform a send transaction 3. See how the Estimate fees and max fees are the same 4. Checkout this branch 5. Build and launch MM 6. Perform a send transaction 7. See how the estimate fees and max fees are not the same and estimate is less than max ## **Screenshots/Recordings** ### **Before** https://github.com/MetaMask/metamask-extension/assets/44811/9f553918-e56b-4a01-a119-7c67ff1593ca ### **After** https://github.com/MetaMask/metamask-extension/assets/44811/883e808b-a85f-43fe-a03a-6df9556b66d3 ## **Pre-merge author checklist** - [x] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've clearly explained what problem this PR is solving and how it is solved. - [x] I've linked related issues - [x] I've included manual testing steps - [x] I've included screenshots/recordings if applicable - [ ] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. - [x] I’ve properly set the pull request status: - [ ] In case it's not yet "ready for review", I've set it to "draft". - [x] 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. --- .../__snapshots__/confirm-gas-display.test.js.snap | 8 ++++---- .../gas-details-item/gas-details-item.js | 14 ++++++++++++-- .../confirm-transaction-base.component.js | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/ui/pages/confirmations/components/confirm-gas-display/__snapshots__/confirm-gas-display.test.js.snap b/ui/pages/confirmations/components/confirm-gas-display/__snapshots__/confirm-gas-display.test.js.snap index e4b206933e3d..99e089514026 100644 --- a/ui/pages/confirmations/components/confirm-gas-display/__snapshots__/confirm-gas-display.test.js.snap +++ b/ui/pages/confirmations/components/confirm-gas-display/__snapshots__/confirm-gas-display.test.js.snap @@ -42,12 +42,12 @@ exports[`ConfirmGasDisplay should match snapshot 1`] = `
- 0.00147 + 0.001197
@@ -60,12 +60,12 @@ exports[`ConfirmGasDisplay should match snapshot 1`] = ` >
- 0.00147 + 0.001197 { + if (isMultiLayerFeeNetwork) { + return sumHexes(hexMinimumTransactionFee, estimatedL1Fees || 0); + } + + return hexMinimumTransactionFee; + }, [isMultiLayerFeeNetwork, hexMinimumTransactionFee, estimatedL1Fees]); + + const getMaxTransactionFeeTotal = useMemo(() => { if (isMultiLayerFeeNetwork) { return sumHexes(hexMaximumTransactionFee, estimatedL1Fees || 0); } @@ -156,7 +164,9 @@ const GasDetailsItem = ({
diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js index 5c735a0f8e93..ec4b68217fa6 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js @@ -537,7 +537,7 @@ export default class ConfirmTransactionBase extends Component { detailText={ useCurrencyRateCheck && renderTotalDetailText(getTotalAmount()) } - detailTotal={renderTotalMaxAmount(true)} + detailTotal={renderTotalMaxAmount(false)} subTitle={t('transactionDetailGasTotalSubtitle')} subText={
From 16a12080e4a05e7401162ea03c679cc75bfe912b Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Fri, 15 Mar 2024 07:56:36 -0230 Subject: [PATCH 2/4] fix: Fix confirmation tx totals on layer 2 networks and for erc20 transfers (#23511) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** While testing https://github.com/MetaMask/metamask-extension/pull/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](https://github.com/MetaMask/metamask-extension/assets/7499938/1d80ba80-3e54-4a94-97e6-daa0f17b8fd7) ### **After: Layer 2 total fees** ![Screenshot from 2024-03-14 23-46-02](https://github.com/MetaMask/metamask-extension/assets/7499938/3fe61964-df99-4c46-af3c-c49aaf9f0c0f) ### **Before: ERC20 sends** ![Screenshot from 2024-03-14 23-59-29](https://github.com/MetaMask/metamask-extension/assets/7499938/e132e060-92c5-4a69-84e9-8fb6c7da2d99) ### **After: ERC20 sends** ![Screenshot from 2024-03-15 00-15-03](https://github.com/MetaMask/metamask-extension/assets/7499938/4d25dca9-c493-478a-84ce-c3b04b1c7a58) ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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. --- .../confirm-transaction-base.component.js | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js index ec4b68217fa6..5ff6bd63e59c 100644 --- a/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js +++ b/ui/pages/confirmations/confirm-transaction-base/confirm-transaction-base.component.js @@ -7,6 +7,8 @@ import { import ConfirmPageContainer from '../components/confirm-page-container'; import { isBalanceSufficient } from '../send/send.utils'; import { DEFAULT_ROUTE } from '../../../helpers/constants/routes'; +import fetchEstimatedL1Fee from '../../../helpers/utils/optimism/fetchEstimatedL1Fee'; + import { INSUFFICIENT_FUNDS_ERROR_KEY, GAS_LIMIT_TOO_LOW_ERROR_KEY, @@ -47,7 +49,7 @@ import { MIN_GAS_LIMIT_DEC } from '../send/send.constants'; import { NETWORK_TO_NAME_MAP } from '../../../../shared/constants/network'; import { - addHexes, + sumHexes, hexToDecimal, } from '../../../../shared/modules/conversion.utils'; import TransactionAlerts from '../components/transaction-alerts'; @@ -159,6 +161,7 @@ export default class ConfirmTransactionBase extends Component { isUserOpContractDeployError: PropTypes.bool, useMaxValue: PropTypes.bool, maxValue: PropTypes.string, + isMultiLayerFeeNetwork: PropTypes.bool, }; state = { @@ -169,6 +172,7 @@ export default class ConfirmTransactionBase extends Component { editingGas: false, userAcknowledgedGasMissing: false, showWarningModal: false, + estimatedL1Fees: 0, ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) noteText: '', ///: END:ONLY_INCLUDE_IF @@ -188,6 +192,7 @@ export default class ConfirmTransactionBase extends Component { setDefaultHomeActiveTabName, hexMaximumTransactionFee, useMaxValue, + txData, } = this.props; const { customNonceValue: prevCustomNonceValue, @@ -242,11 +247,21 @@ export default class ConfirmTransactionBase extends Component { } } - if ( - hexMaximumTransactionFee !== prevHexMaximumTransactionFee && - useMaxValue - ) { - this.updateValueToMax(); + if (hexMaximumTransactionFee !== prevHexMaximumTransactionFee) { + fetchEstimatedL1Fee(txData?.chainId, txData) + .then((result) => { + this.setState({ + estimatedL1Fees: result, + }); + }) + .catch((_err) => { + this.setState({ + estimatedL1Fees: 0, + }); + }); + if (useMaxValue) { + this.updateValueToMax(); + } } } @@ -377,10 +392,11 @@ export default class ConfirmTransactionBase extends Component { useCurrencyRateCheck, tokenSymbol, isUsingPaymaster, + isMultiLayerFeeNetwork, } = this.props; const { t } = this.context; - const { userAcknowledgedGasMissing } = this.state; + const { userAcknowledgedGasMissing, estimatedL1Fees } = this.state; const { valid } = this.getErrorKey(); const isDisabled = () => { @@ -394,9 +410,10 @@ export default class ConfirmTransactionBase extends Component { const networkName = NETWORK_TO_NAME_MAP[txData.chainId]; const getTotalAmount = (useMaxFee) => { - return addHexes( + return sumHexes( txData.txParams.value, useMaxFee ? hexMaximumTransactionFee : hexMinimumTransactionFee, + isMultiLayerFeeNetwork ? estimatedL1Fees : 0, ); }; @@ -417,8 +434,11 @@ export default class ConfirmTransactionBase extends Component { } // Token send - return useNativeCurrencyAsPrimaryCurrency + const primaryTotal = useMaxFee ? primaryTotalTextOverrideMaxAmount + : primaryTotalTextOverride; + return useNativeCurrencyAsPrimaryCurrency + ? primaryTotal : secondaryTotalTextOverride; }; From 7d36bbf3fd0227321a6a5758538e7fbe27044c18 Mon Sep 17 00:00:00 2001 From: Dan J Miller Date: Thu, 14 Mar 2024 22:02:55 -0230 Subject: [PATCH 3/4] v11.12.2 --- CHANGELOG.md | 9 ++++++++- package.json | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d9503370954e..da035b6c047d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [11.12.2] +### Fixed +- Fix transaction confirmations so that they correctly show estimated fees instead of max possible fees + - For non-layer 2 network, and non-token send, transactions([#23203](https://github.com/MetaMask/metamask-extension/pull/23203)) + - For layer 2 networks and for erc20 transfers ([#23511](https://github.com/MetaMask/metamask-extension/pull/23511)) + ## [11.12.1] ### Changed - Updated styling for missing petnames to prevent misinterpretation as malicious ([#23458](https://github.com/MetaMask/metamask-extension/pull/23458)) @@ -4496,7 +4502,8 @@ Update styles and spacing on the critical error page ([#20350](https://github.c ### Uncategorized - Added the ability to restore accounts from seed words. -[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v11.12.1...HEAD +[Unreleased]: https://github.com/MetaMask/metamask-extension/compare/v11.12.2...HEAD +[11.12.2]: https://github.com/MetaMask/metamask-extension/compare/v11.12.1...v11.12.2 [11.12.1]: https://github.com/MetaMask/metamask-extension/compare/v11.12.0...v11.12.1 [11.12.0]: https://github.com/MetaMask/metamask-extension/compare/v11.11.4...v11.12.0 [11.11.4]: https://github.com/MetaMask/metamask-extension/compare/v11.11.3...v11.11.4 diff --git a/package.json b/package.json index b9fbd226cb8b..749d7272c51b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "metamask-crx", - "version": "11.12.1", + "version": "11.12.2", "private": true, "repository": { "type": "git", From 339f24a76dad67b5b667d6f1aa10141c67c312ee Mon Sep 17 00:00:00 2001 From: Jyoti Puri Date: Fri, 15 Mar 2024 23:47:27 +0530 Subject: [PATCH 4/4] fix: Gas display fixes on confirmation screen (#23524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR corrects the display of the "Total" value under the "Fee Details" dropdown. It also ensures the "Fee details" dropdown is only shown on layer 2 networks. This is the correct spec, as confirmed with @bschorchit Fixes: https://github.com/MetaMask/metamask-extension/issues/23515 1. On eth mainnet, create a transaction 2. There should be no "Fee details" dropdown, and other fee information should be correct. 1. On Optimism, create a transaction 2. Their should be a fee details, and opening it should show a Total that matches the estimated total shown below/outside the "fee details" section Layer 1 ![Screenshot from 2024-03-15 14-08-03](https://github.com/MetaMask/metamask-extension/assets/7499938/ed348930-14db-4818-b207-9f3fa2c09ff6) Layer 2 ![Screenshot from 2024-03-15 14-07-10](https://github.com/MetaMask/metamask-extension/assets/7499938/bb879810-87bc-42cc-9bf5-4860aa6e5b75) Layer 1 ![Screenshot from 2024-03-15 14-09-44](https://github.com/MetaMask/metamask-extension/assets/7499938/cce1d206-088c-4b60-889c-ed467e53b535) Layer 2 ![Screenshot from 2024-03-15 13-59-02](https://github.com/MetaMask/metamask-extension/assets/7499938/e8d41718-e10a-4f3e-873c-e55a64bed8b9) - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [ ] 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](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). 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". - [ ] 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. --------- Co-authored-by: Dan J Miller --- .../fee-details-component.js | 13 +-- .../fee-details-component.test.js | 50 +++++++-- ...irm-approve-content.component.test.js.snap | 104 +----------------- .../confirm-send-ether.test.js.snap | 26 +---- .../confirm-transaction-base.test.js.snap | 26 +---- 5 files changed, 51 insertions(+), 168 deletions(-) diff --git a/ui/pages/confirmations/components/fee-details-component/fee-details-component.js b/ui/pages/confirmations/components/fee-details-component/fee-details-component.js index b329d6704951..0c84336bed3e 100644 --- a/ui/pages/confirmations/components/fee-details-component/fee-details-component.js +++ b/ui/pages/confirmations/components/fee-details-component/fee-details-component.js @@ -43,10 +43,7 @@ export default function FeeDetailsComponent({ const t = useI18nContext(); - const { - maximumCostInHexWei: hexMaximumTransactionFee, - minimumCostInHexWei: hexMinimumTransactionFee, - } = useGasFeeContext(); + const { minimumCostInHexWei: hexMinimumTransactionFee } = useGasFeeContext(); useEffect(() => { if (isMultiLayerFeeNetwork) { fetchEstimatedL1Fee(txData?.chainId, txData) @@ -61,11 +58,11 @@ export default function FeeDetailsComponent({ const getTransactionFeeTotal = useMemo(() => { if (isMultiLayerFeeNetwork) { - return addHexes(hexMaximumTransactionFee, estimatedL1Fees || 0); + return addHexes(hexMinimumTransactionFee, estimatedL1Fees || 0); } - return hexMaximumTransactionFee; - }, [isMultiLayerFeeNetwork, hexMaximumTransactionFee, estimatedL1Fees]); + return hexMinimumTransactionFee; + }, [isMultiLayerFeeNetwork, hexMinimumTransactionFee, estimatedL1Fees]); const renderTotalDetailText = useCallback( (value) => { @@ -109,7 +106,7 @@ export default function FeeDetailsComponent({ justifyContent={JustifyContent.center} flexDirection={FlexDirection.Column} > - {!hideGasDetails && ( + {!hideGasDetails && isMultiLayerFeeNetwork && ( { }; describe('FeeDetailsComponent', () => { - it('renders "Fee details"', () => { - render(); + it('renders "Fee details"', async () => { + await render({ + ...mockState, + metamask: { + ...mockState.metamask, + providerConfig: { + chainId: CHAIN_IDS.OPTIMISM, + }, + }, + }); expect(screen.queryByText('Fee details')).toBeInTheDocument(); }); - it('should expand when button is clicked', () => { - render(); + it('should expand when button is clicked', async () => { + await render({ + ...mockState, + metamask: { + ...mockState.metamask, + providerConfig: { + chainId: CHAIN_IDS.OPTIMISM, + }, + }, + }); expect(screen.queryByTitle('0 ETH')).not.toBeInTheDocument(); - screen.getByRole('button').click(); - expect(screen.queryByTitle('0 ETH')).toBeInTheDocument(); + await act(async () => { + screen.getByRole('button').click(); + }); + expect(screen.getAllByTitle('0 ETH')).toHaveLength(2); + expect(screen.getAllByTitle('0 ETH')[0]).toBeInTheDocument(); }); - it('should be displayed for even legacy network', () => { - render({ + it('should be displayed for layer 2 network', async () => { + await render({ ...mockState, metamask: { ...mockState.metamask, @@ -41,6 +60,17 @@ describe('FeeDetailsComponent', () => { 1559: false, }, }, + networksMetadata: { + goerli: { + EIPS: { + 1559: false, + }, + status: 'available', + }, + }, + providerConfig: { + chainId: CHAIN_IDS.OPTIMISM, + }, }, }); expect(screen.queryByText('Fee details')).toBeInTheDocument(); diff --git a/ui/pages/confirmations/confirm-approve/confirm-approve-content/__snapshots__/confirm-approve-content.component.test.js.snap b/ui/pages/confirmations/confirm-approve/confirm-approve-content/__snapshots__/confirm-approve-content.component.test.js.snap index 84374e307205..a4bd02de6c35 100644 --- a/ui/pages/confirmations/confirm-approve/confirm-approve-content/__snapshots__/confirm-approve-content.component.test.js.snap +++ b/ui/pages/confirmations/confirm-approve/confirm-approve-content/__snapshots__/confirm-approve-content.component.test.js.snap @@ -115,31 +115,7 @@ exports[`ConfirmApproveContent Component should render Confirm approve page corr >
-
- -
-
+ />
-
- -
-
+ />
-
- -
-
+ />
-
- -
-
+ />
-
- -
-
+ />
diff --git a/ui/pages/confirmations/confirm-transaction-base/__snapshots__/confirm-transaction-base.test.js.snap b/ui/pages/confirmations/confirm-transaction-base/__snapshots__/confirm-transaction-base.test.js.snap index 2f64281591fc..6fde90864dde 100644 --- a/ui/pages/confirmations/confirm-transaction-base/__snapshots__/confirm-transaction-base.test.js.snap +++ b/ui/pages/confirmations/confirm-transaction-base/__snapshots__/confirm-transaction-base.test.js.snap @@ -405,31 +405,7 @@ exports[`Confirm Transaction Base should match snapshot 1`] = `
-
- -
-
+ />