Skip to content

Commit e659f4f

Browse files
fix: Missing "Unlimited" as value for the DAI permit (#29597)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> ## **Description** Introduces a new logical path for displaying "Unlimited" in the permit simulation, for when the `allowed` property in the signature message is set and the permit is for the token DAI. This is as is specified in [ERC-2612](https://eips.ethereum.org/EIPS/eip-2612)'s "Backwards Compatibility" section: > There are already a couple of permit functions in token contracts implemented in contracts in the wild, most notably the one introduced in the dai.sol. > Its implementation differs slightly from the presentation here in that: > instead of taking a value argument, it takes a bool allowed, setting approval to 0 or uint(-1). This PR also fixes a bug that prevents boolean values from being displayed in the key value display in the message section of signatures. <!-- 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? --> [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/29597?quickstart=1) ## **Related issues** Fixes: #28964 ## **Manual testing steps** 1. Open the browser console and execute the following code: ```javascript async function connectMetaMask() { try { const accounts = await window.ethereum.request({ method: 'eth_requestAccounts', }); console.log('Connected account:', accounts[0]); return accounts[0]; } catch (error) { console.error('User rejected the request:', error); throw error; } } async function signPermit() { try { const fromAddress = await connectMetaMask(); const msgParams = { types: { EIP712Domain: [ { name: 'name', type: 'string' }, { name: 'version', type: 'string' }, { name: 'chainId', type: 'uint256' }, { name: 'verifyingContract', type: 'address' }, ], Permit: [ { name: 'holder', type: 'address' }, { name: 'spender', type: 'address' }, { name: 'nonce', type: 'uint256' }, { name: 'expiry', type: 'uint256' }, { name: 'allowed', type: 'bool' }, ], }, domain: { name: 'Dai Stablecoin', version: '1', verifyingContract: '0x6B175474E89094C44Da98b954EedeAC495271d0F', chainId: '0x1', }, primaryType: 'Permit', message: { holder: '0xD2C44F28eC4C7eF686f587FADdb204da3aEFa827', spender: '0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45', allowed: true, nonce: 0, expiry: 1660916504, }, }; const signature = await window.ethereum.request({ method: 'eth_signTypedData_v4', params: [fromAddress, JSON.stringify(msgParams)], }); console.log('Signature:', signature); return signature; } catch (error) { console.error('Error signing permit:', error); } } signPermit(); ``` 2. Change `"allowed": true` to `"allowed": false` in the aforementioned code and execute it to see the revocation screen. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> <img width="472" alt="Screenshot 2025-01-08 at 18 30 35" src="https://github.com/user-attachments/assets/93e5f295-7b75-4e1d-88c0-aaa1d11ff01d" /> <img width="472" alt="Screenshot 2025-01-08 at 18 30 37" src="https://github.com/user-attachments/assets/59e8b129-df26-4f24-bcd2-a978f974eedd" /> <img width="472" alt="Screenshot 2025-01-09 at 10 22 09" src="https://github.com/user-attachments/assets/10064bde-2e65-43de-b624-1c0a04d6eea4" /> <img width="472" alt="Screenshot 2025-01-09 at 10 22 13" src="https://github.com/user-attachments/assets/22ecff23-71de-4293-9fba-fab74b52a42c" /> ## **Pre-merge author checklist** - [ ] 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). - [ ] I've completed the PR template to the best of my ability - [ ] 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/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.
1 parent 16ac089 commit e659f4f

File tree

10 files changed

+105
-272
lines changed

10 files changed

+105
-272
lines changed

test/integration/confirmations/signatures/permit-batch.test.tsx

+2-2
Original file line numberDiff line numberDiff line change
@@ -103,9 +103,9 @@ describe('Permit Batch Signature Tests', () => {
103103
"You're giving the spender permission to spend this many tokens from your account.",
104104
'Spending cap',
105105
'0xA0b86...6eB48',
106-
'1,461,501,637,3...',
106+
'Unlimited',
107107
'0xb0B86...6EB48',
108-
'2,461,501,637,3...',
108+
'Unlimited',
109109
];
110110

111111
verifyDetails(simulationSection, simulationDetails);

test/integration/confirmations/signatures/permit-single.test.tsx

+1-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ describe('Permit Single Signature Tests', () => {
101101
"You're giving the spender permission to spend this many tokens from your account.",
102102
'Spending cap',
103103
'0xA0b86...6eB48',
104-
'1,461,501,637,3...',
104+
'Unlimited',
105105
];
106106

107107
expect(simulationSection).toBeInTheDocument();
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
11
export const HEX_ZERO = '0x0';
22

33
export const TOKEN_VALUE_UNLIMITED_THRESHOLD = 10 ** 15;
4+
5+
export const DAI_CONTRACT_ADDRESS =
6+
'0x6B175474E89094C44Da98b954EedeAC495271d0F';

ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign-v4-simulation/permit-simulation/permit-simulation.tsx

+13-3
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,8 @@ const PermitSimulation: React.FC<object> = () => {
6868
tokenContract={token}
6969
value={amount}
7070
chainId={chainId}
71+
message={message}
72+
canDisplayValueAsUnlimited
7173
/>
7274
);
7375

@@ -97,19 +99,27 @@ const PermitSimulation: React.FC<object> = () => {
9799
value={message.value}
98100
tokenId={message.tokenId}
99101
chainId={chainId}
102+
message={message}
103+
canDisplayValueAsUnlimited
100104
/>
101105
)}
102106
</Box>
103107
</ConfirmInfoRow>
104108
);
105109

110+
let descriptionKey = 'permitSimulationDetailInfo';
111+
if (isNFT) {
112+
descriptionKey = 'simulationDetailsApproveDesc';
113+
} else if (message.allowed === false) {
114+
// revoke permit
115+
descriptionKey = 'revokeSimulationDetailsDesc';
116+
}
117+
106118
return (
107119
<StaticSimulation
108120
title={t('simulationDetailsTitle')}
109121
titleTooltip={t('simulationDetailsTitleTooltip')}
110-
description={t(
111-
isNFT ? 'simulationDetailsApproveDesc' : 'permitSimulationDetailInfo',
112-
)}
122+
description={t(descriptionKey)}
113123
simulationElements={SpendingCapRow}
114124
/>
115125
);

ui/pages/confirmations/components/confirm/info/typed-sign/typed-sign-v4-simulation/value-display/value-display.tsx

+18-3
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,10 @@ import {
2828
formatAmount,
2929
formatAmountMaxPrecision,
3030
} from '../../../../../simulation-details/formatAmount';
31-
import { TOKEN_VALUE_UNLIMITED_THRESHOLD } from '../../../shared/constants';
31+
import {
32+
DAI_CONTRACT_ADDRESS,
33+
TOKEN_VALUE_UNLIMITED_THRESHOLD,
34+
} from '../../../shared/constants';
3235
import { getAmountColors } from '../../../utils';
3336

3437
type PermitSimulationValueDisplayParams = {
@@ -51,6 +54,9 @@ type PermitSimulationValueDisplayParams = {
5154
/** The tokenId for NFT */
5255
tokenId?: string;
5356

57+
/** The permit message */
58+
message?: { allowed?: boolean | null };
59+
5460
/** True if value is being credited to wallet */
5561
credit?: boolean;
5662

@@ -69,6 +75,7 @@ const PermitSimulationValueDisplay: React.FC<
6975
tokenContract,
7076
tokenId,
7177
value,
78+
message,
7279
credit,
7380
debit,
7481
canDisplayValueAsUnlimited,
@@ -96,22 +103,30 @@ const PermitSimulationValueDisplay: React.FC<
96103

97104
const { tokenValue, tokenValueMaxPrecision, shouldShowUnlimitedValue } =
98105
useMemo(() => {
106+
const isDAIPermit = tokenContract === DAI_CONTRACT_ADDRESS;
107+
const hasPermitAllowedProp = message?.allowed !== undefined;
108+
const showUnlimitedDueToDAIContract = isDAIPermit && hasPermitAllowedProp;
109+
99110
if (!value || tokenId) {
100111
return {
101112
tokenValue: null,
102113
tokenValueMaxPrecision: null,
103-
shouldShowUnlimitedValue: false,
114+
shouldShowUnlimitedValue:
115+
canDisplayValueAsUnlimited && showUnlimitedDueToDAIContract,
104116
};
105117
}
106118

107119
const tokenAmount = calcTokenAmount(value, tokenDecimals);
108120

121+
const showUnlimitedDueToPermitValue =
122+
Number(value) > TOKEN_VALUE_UNLIMITED_THRESHOLD;
123+
109124
return {
110125
tokenValue: formatAmount('en-US', tokenAmount),
111126
tokenValueMaxPrecision: formatAmountMaxPrecision('en-US', tokenAmount),
112127
shouldShowUnlimitedValue:
113128
canDisplayValueAsUnlimited &&
114-
Number(value) > TOKEN_VALUE_UNLIMITED_THRESHOLD,
129+
(showUnlimitedDueToPermitValue || showUnlimitedDueToDAIContract),
115130
};
116131
}, [tokenDecimals, tokenId, value]);
117132

ui/pages/confirmations/components/confirm/row/dataTree.tsx

+4
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,10 @@ const DataField = memo(
201201
return <ConfirmInfoRowAddress address={value} chainId={chainId} />;
202202
}
203203

204+
if (type === 'bool') {
205+
return <ConfirmInfoRowText text={String(value)} />;
206+
}
207+
204208
return <ConfirmInfoRowText text={sanitizeString(value)} />;
205209
},
206210
);

ui/pages/confirmations/components/confirm/title/title.tsx

+19-2
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,13 @@ import useAlerts from '../../../../../hooks/useAlerts';
1616
import { useI18nContext } from '../../../../../hooks/useI18nContext';
1717
import { TypedSignSignaturePrimaryTypes } from '../../../constants';
1818
import { useConfirmContext } from '../../../context/confirm';
19+
import { useTypedSignSignatureInfo } from '../../../hooks/useTypedSignSignatureInfo';
1920
import { Confirmation, SignatureRequestType } from '../../../types/confirm';
2021
import { isSIWESignatureRequest } from '../../../utils';
21-
import { useTypedSignSignatureInfo } from '../../../hooks/useTypedSignSignatureInfo';
2222
import { useIsNFT } from '../info/approve/hooks/use-is-nft';
23-
import { getIsRevokeSetApprovalForAll } from '../info/utils';
2423
import { useTokenTransactionData } from '../info/hooks/useTokenTransactionData';
24+
import { getIsRevokeSetApprovalForAll } from '../info/utils';
25+
import { getIsRevokeDAIPermit } from '../utils';
2526
import { useCurrentSpendingCap } from './hooks/useCurrentSpendingCap';
2627

2728
function ConfirmBannerAlert({ ownerId }: { ownerId: string }) {
@@ -84,6 +85,14 @@ const getTitle = (
8485
if (tokenStandard === TokenStandard.ERC721) {
8586
return t('setApprovalForAllRedesignedTitle');
8687
}
88+
89+
const isRevokeDAIPermit = getIsRevokeDAIPermit(
90+
confirmation as SignatureRequestType,
91+
);
92+
if (isRevokeDAIPermit) {
93+
return t('confirmTitleRevokeApproveTransaction');
94+
}
95+
8796
return t('confirmTitlePermitTokens');
8897
}
8998
return t('confirmTitleSignature');
@@ -136,6 +145,14 @@ const getDescription = (
136145
if (tokenStandard === TokenStandard.ERC721) {
137146
return t('confirmTitleDescApproveTransaction');
138147
}
148+
149+
const isRevokeDAIPermit = getIsRevokeDAIPermit(
150+
confirmation as SignatureRequestType,
151+
);
152+
if (isRevokeDAIPermit) {
153+
return '';
154+
}
155+
139156
return t('confirmTitleDescPermitSignature');
140157
}
141158
return t('confirmTitleDescSign');

ui/pages/confirmations/components/confirm/utils.ts

+14
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
import { TransactionMeta } from '@metamask/transaction-controller';
2+
import { parseTypedDataMessage } from '../../../../../shared/modules/transaction.utils';
23
import { Confirmation, SignatureRequestType } from '../../types/confirm';
4+
import { DAI_CONTRACT_ADDRESS } from './info/shared/constants';
35

46
export const getConfirmationSender = (
57
currentConfirmation: Confirmation | undefined,
@@ -28,3 +30,15 @@ export const formatNumber = (value: number, decimals: number) => {
2830
});
2931
return formatter.format(value);
3032
};
33+
34+
export const getIsRevokeDAIPermit = (confirmation: SignatureRequestType) => {
35+
const msgData = confirmation?.msgParams?.data;
36+
const {
37+
message,
38+
domain: { verifyingContract },
39+
} = parseTypedDataMessage(msgData as string);
40+
const isRevokeDAIPermit =
41+
message.allowed === false && verifyingContract === DAI_CONTRACT_ADDRESS;
42+
43+
return isRevokeDAIPermit;
44+
};

0 commit comments

Comments
 (0)