Skip to content

Commit

Permalink
fix (cherry-pick): remove reliance on transaction decode in confirmat…
Browse files Browse the repository at this point in the history
…ions #29341 (#29397)

## **Description**

Cherry-pick of #29341 for release 12.9.3.

Difference to `main` is that redesigned transactions are disabled if the
`Decode smart contracts` toggle is disabled.

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

## **Related issues**

## **Manual testing steps**

## **Screenshots/Recordings**

### **Before**

### **After**

## **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 317b923 commit 1712e28
Show file tree
Hide file tree
Showing 6 changed files with 84 additions and 1 deletion.
22 changes: 22 additions & 0 deletions shared/lib/confirmation.utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,17 @@ describe('confirmation.utils', () => {
}),
).toBe(false);
});

it('should return false if decoding is disabled', () => {
expect(
shouldUseRedesignForTransactions({
transactionMetadataType: unsupportedTransactionType,
isRedesignedTransactionsUserSettingEnabled: true, // user setting enabled
isRedesignedConfirmationsDeveloperEnabled: false, // developer mode disabled
isDecodingEnabled: false,
}),
).toBe(false);
});
});

describe('when developer mode is enabled', () => {
Expand Down Expand Up @@ -93,6 +104,17 @@ describe('confirmation.utils', () => {
}),
).toBe(false);
});

it('should return false if decoding is disabled', () => {
expect(
shouldUseRedesignForTransactions({
transactionMetadataType: unsupportedTransactionType,
isRedesignedTransactionsUserSettingEnabled: false, // user setting disabled
isRedesignedConfirmationsDeveloperEnabled: true, // developer mode enabled
isDecodingEnabled: false,
}),
).toBe(false);
});
});

describe('when both user setting and developer mode are disabled', () => {
Expand Down
6 changes: 6 additions & 0 deletions shared/lib/confirmation.utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,17 @@ export function shouldUseRedesignForTransactions({
transactionMetadataType,
isRedesignedTransactionsUserSettingEnabled,
isRedesignedConfirmationsDeveloperEnabled,
isDecodingEnabled,
}: {
transactionMetadataType?: TransactionType;
isRedesignedTransactionsUserSettingEnabled: boolean;
isRedesignedConfirmationsDeveloperEnabled: boolean;
isDecodingEnabled?: boolean;
}): boolean {
if (isDecodingEnabled === false) {
return false;
}

return (
shouldUseRedesignForTransactionsUserMode(
isRedesignedTransactionsUserSettingEnabled,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,32 @@ describe('useDecodedTransactionData', () => {
expect(result).toStrictEqual({ pending: false, value: undefined });
});

it('returns undefined if decode disabled', async () => {
decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY);

const result = await runHook(
getMockConfirmStateForTransaction(
{
id: '123',
chainId: CHAIN_ID_MOCK,
type: TransactionType.contractInteraction,
status: TransactionStatus.unapproved,
txParams: {
data: TRANSACTION_DATA_UNISWAP,
to: CONTRACT_ADDRESS_MOCK,
} as TransactionParams,
},
{
metamask: {
use4ByteResolution: false,
},
},
),
);

expect(result).toStrictEqual({ pending: false, value: undefined });
});

it('returns the decoded data', async () => {
decodeTransactionDataMock.mockResolvedValue(TRANSACTION_DECODE_SOURCIFY);

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Hex } from '@metamask/utils';
import { TransactionMeta } from '@metamask/transaction-controller';

import { useSelector } from 'react-redux';
import {
AsyncResult,
useAsyncResult,
Expand All @@ -9,11 +10,13 @@ import { decodeTransactionData } from '../../../../../../store/actions';
import { DecodedTransactionDataResponse } from '../../../../../../../shared/types/transaction-decode';
import { useConfirmContext } from '../../../../context/confirm';
import { hasTransactionData } from '../../../../../../../shared/modules/transaction.utils';
import { use4ByteResolutionSelector } from '../../../../../../selectors';

export function useDecodedTransactionData(
transactionTypeFilter?: string,
): AsyncResult<DecodedTransactionDataResponse | undefined> {
const { currentConfirmation } = useConfirmContext<TransactionMeta>();
const isDecodeEnabled = useSelector(use4ByteResolutionSelector);

const currentTransactionType = currentConfirmation?.type;
const chainId = currentConfirmation?.chainId as Hex;
Expand All @@ -23,6 +26,7 @@ export function useDecodedTransactionData(

return useAsyncResult(async () => {
if (
!isDecodeEnabled ||
!hasTransactionData(transactionData) ||
!transactionTo ||
(transactionTypeFilter &&
Expand All @@ -36,5 +40,11 @@ export function useDecodedTransactionData(
chainId,
contractAddress,
});
}, [transactionData, transactionTo, chainId, contractAddress]);
}, [
isDecodeEnabled,
transactionData,
transactionTo,
chainId,
contractAddress,
]);
}
16 changes: 16 additions & 0 deletions ui/pages/confirmations/hooks/useCurrentConfirmation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ function buildState({
redesignedTransactionsEnabled,
transaction,
isRedesignedConfirmationsDeveloperEnabled,
isDecodingEnabled,
}: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
message?: Partial<AbstractMessage & { msgParams: any }>;
Expand All @@ -67,6 +68,7 @@ function buildState({
redesignedTransactionsEnabled?: boolean;
transaction?: Partial<TransactionMeta>;
isRedesignedConfirmationsDeveloperEnabled?: boolean;
isDecodingEnabled?: boolean;
}) {
return {
...mockState,
Expand All @@ -83,6 +85,7 @@ function buildState({
unapprovedPersonalMsgs: message
? { [message.id as string]: message }
: {},
use4ByteResolution: isDecodingEnabled ?? true,
},
};
}
Expand Down Expand Up @@ -290,6 +293,19 @@ describe('useCurrentConfirmation', () => {
expect(currentConfirmation).toStrictEqual(TRANSACTION_MOCK);
});

it('returns undefined if transaction type correct and redesign enabled but decoding disabled', () => {
const currentConfirmation = runHook({
pendingApprovals: [{ ...APPROVAL_MOCK, type: ApprovalType.Transaction }],
redesignedConfirmationsEnabled: true,
transaction: TRANSACTION_MOCK,
redesignedTransactionsEnabled: true,
isRedesignedConfirmationsDeveloperEnabled: false,
isDecodingEnabled: false,
});

expect(currentConfirmation).toBeUndefined();
});

describe('useCurrentConfirmation with env var', () => {
beforeAll(() => {
jest.resetModules();
Expand Down
3 changes: 3 additions & 0 deletions ui/pages/confirmations/hooks/useCurrentConfirmation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
getUnapprovedTransaction,
oldestPendingConfirmationSelector,
selectPendingApproval,
use4ByteResolutionSelector,
} from '../../../selectors';
import { selectUnapprovedMessage } from '../../../selectors/signatures';
import {
Expand All @@ -30,6 +31,7 @@ const useCurrentConfirmation = () => {
const { id: paramsConfirmationId } = useParams<{ id: string }>();
const oldestPendingApproval = useSelector(oldestPendingConfirmationSelector);
const confirmationId = paramsConfirmationId ?? oldestPendingApproval?.id;
const isDecodingEnabled = Boolean(useSelector(use4ByteResolutionSelector));

const isRedesignedSignaturesUserSettingEnabled = useSelector(
getRedesignedConfirmationsEnabled,
Expand Down Expand Up @@ -66,6 +68,7 @@ const useCurrentConfirmation = () => {
transactionMetadataType: transactionMetadata?.type,
isRedesignedTransactionsUserSettingEnabled,
isRedesignedConfirmationsDeveloperEnabled,
isDecodingEnabled,
});

const shouldUseRedesign =
Expand Down

0 comments on commit 1712e28

Please sign in to comment.