Skip to content

Commit

Permalink
fix: remove reliance on transaction decode in confirmations (#29341)
Browse files Browse the repository at this point in the history
## **Description**

Disable all advanced transaction data decoding using Sourcify, 4Byte and
Uniswap, if the `Decode smart contracts` toggle is disabled.

Remove all reliance on the advanced decoding excluding the `Data`
section.

Specifically: 

- Create `useTokenTransactionData` hook to decode all token transactions
locally using the ABIs.
- Replace all usages of `useDecodedTransactionData` with the new hook,
except for the `TransactionData` component.
- Update related unit and integration tests to decode valid test data
rather than rely on mocking hooks.

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29341?quickstart=1)

## **Related issues**

## **Manual testing steps**

Regression of redesigned transaction confirmations.

Specifically:

- Token Transfer Recipient
- Token Transfer Amount
- Approval Spender
- Approval Spending Cap

## **Screenshots/Recordings**

### **Before**

### **After**

<img width="354" alt="New Toggle Description"
src="https://github.com/user-attachments/assets/180e23d4-39d8-44b3-b3cf-38bed360ec9e"
/>

## **Pre-merge author checklist**

- [x] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've completed the PR template to the best of my ability
- [x] 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/main/.github/guidelines/LABELING_GUIDELINES.md)).
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.
  • Loading branch information
matthewwalsh0 authored Dec 20, 2024
1 parent 2eced17 commit 1c6391c
Show file tree
Hide file tree
Showing 52 changed files with 1,542 additions and 671 deletions.
3 changes: 0 additions & 3 deletions app/_locales/de/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/el/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions app/_locales/en/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/es/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/fr/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/hi/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/id/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/ja/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/ko/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/pt/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/ru/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/tl/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/tr/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/vi/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 0 additions & 3 deletions app/_locales/zh_CN/messages.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion test/data/confirmations/set-approval-for-all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import {
genUnapprovedContractInteractionConfirmation,
} from './contract-interaction';

export const INCREASE_ALLOWANCE_TRANSACTION_DATA =
'0x395093510000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b0000000000000000000000000000000000000000000000000000000000000123';

export const genUnapprovedSetApprovalForAllConfirmation = ({
address = CONTRACT_INTERACTION_SENDER_ADDRESS,
chainId = CHAIN_ID,
Expand All @@ -16,7 +19,7 @@ export const genUnapprovedSetApprovalForAllConfirmation = ({
...genUnapprovedContractInteractionConfirmation({ chainId }),
txParams: {
from: address,
data: '0x095ea7b30000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b0000000000000000000000000000000000000000000000000000000000000001',
data: '0xa22cb4650000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b0000000000000000000000000000000000000000000000000000000000000001',
gas: '0x16a92',
to: '0x076146c765189d51be3160a2140cf80bfc73ad68',
value: '0x0',
Expand Down
4 changes: 3 additions & 1 deletion test/data/confirmations/token-approve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import {
export const genUnapprovedApproveConfirmation = ({
address = CONTRACT_INTERACTION_SENDER_ADDRESS,
chainId = CHAIN_ID,
amountHex = '0000000000000000000000000000000000000000000000000000000000000001',
}: {
address?: Hex;
chainId?: string;
amountHex?: string;
} = {}) => ({
...genUnapprovedContractInteractionConfirmation({ chainId }),
txParams: {
from: address,
data: '0x095ea7b30000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b0000000000000000000000000000000000000000000000000000000000000001',
data: `0x095ea7b30000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b${amountHex}`,
gas: '0x16a92',
to: '0x076146c765189d51be3160a2140cf80bfc73ad68',
value: '0x0',
Expand Down
7 changes: 6 additions & 1 deletion test/data/confirmations/token-transfer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,24 @@ import {
genUnapprovedContractInteractionConfirmation,
} from './contract-interaction';

export const TRANSFER_FROM_TRANSACTION_DATA =
'0x23b872dd0000000000000000000000002e0D7E8c45221FcA00d74a3609A0f7097035d09B0000000000000000000000002e0D7E8c45221FcA00d74a3609A0f7097035d09C0000000000000000000000000000000000000000000000000000000000000123';

export const genUnapprovedTokenTransferConfirmation = ({
address = CONTRACT_INTERACTION_SENDER_ADDRESS,
chainId = CHAIN_ID,
isWalletInitiatedConfirmation = false,
amountHex = '0000000000000000000000000000000000000000000000000000000000000001',
}: {
address?: Hex;
chainId?: string;
isWalletInitiatedConfirmation?: boolean;
amountHex?: string;
} = {}) => ({
...genUnapprovedContractInteractionConfirmation({ chainId }),
txParams: {
from: address,
data: '0x095ea7b30000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b0000000000000000000000000000000000000000000000000000000000000001',
data: `0xa9059cbb0000000000000000000000002e0d7e8c45221fca00d74a3609a0f7097035d09b${amountHex}`,
gas: '0x16a92',
to: '0x076146c765189d51be3160a2140cf80bfc73ad68',
value: '0x0',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ describe('ERC20 increaseAllowance Confirmation', () => {
'simulation-token-value',
);
expect(simulationSection).toContainElement(spendingCapValue);
expect(spendingCapValue).toHaveTextContent('1');
expect(spendingCapValue).toHaveTextContent('3');
expect(simulationSection).toHaveTextContent('0x07614...3ad68');
});

Expand All @@ -240,7 +240,7 @@ describe('ERC20 increaseAllowance Confirmation', () => {

expect(approveDetails).toContainElement(approveDetailsSpender);
expect(approveDetailsSpender).toHaveTextContent(tEn('spender') as string);
expect(approveDetailsSpender).toHaveTextContent('0x2e0D7...5d09B');
expect(approveDetailsSpender).toHaveTextContent('0x9bc5b...AfEF4');
const spenderTooltip = await screen.findByTestId(
'confirmation__approve-spender-tooltip',
);
Expand Down Expand Up @@ -301,7 +301,7 @@ describe('ERC20 increaseAllowance Confirmation', () => {
);
expect(spendingCapSection).toContainElement(spendingCapGroup);
expect(spendingCapGroup).toHaveTextContent(tEn('spendingCap') as string);
expect(spendingCapGroup).toHaveTextContent('1');
expect(spendingCapGroup).toHaveTextContent('3');

const spendingCapGroupTooltip = await screen.findByTestId(
'confirmation__approve-spending-cap-group-tooltip',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ describe('ERC721 setApprovalForAll Confirmation', () => {
expect(approveDetailsSpender).toHaveTextContent(
tEn('permissionFor') as string,
);
expect(approveDetailsSpender).toHaveTextContent('0x2e0D7...5d09B');
expect(approveDetailsSpender).toHaveTextContent('0x9bc5b...AfEF4');
const spenderTooltip = await screen.findByTestId(
'confirmation__approve-spender-tooltip',
);
Expand Down
2 changes: 1 addition & 1 deletion ui/helpers/constants/settings.js
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ const SETTINGS_CONSTANTS = [
{
tabMessage: (t) => t('securityAndPrivacy'),
sectionMessage: (t) => t('use4ByteResolution'),
descriptionMessage: (t) => t('use4ByteResolutionDescription'),
descriptionMessage: (t) => t('toggleDecodeDescription'),
route: `${SECURITY_ROUTE}#decode-smart-contracts`,
icon: 'fa fa-lock',
},
Expand Down
Loading

0 comments on commit 1c6391c

Please sign in to comment.