Skip to content

Commit f8392ec

Browse files
fix(typed-sign): align DAI permit UI with signer coercion and correct hex expiry display (#25789)
<!-- 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** <!-- 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? --> ## Summary This PR fixes a typed-sign confirmation mismatch for legacy DAI Permit by aligning UI interpretation with `@metamask/eth-sig- util` coercion behavior. ## Problem - The UI previously treated `allowed: 'false'` (string) as a revoke. - The signer encodes `bool` using JavaScript coercion (`Boolean(value)`), so `'false'` is truthy and signs as `true`. - Date-like permit fields (such as `expiry`) were parsed with `parseInt(value, 10)`, which can misrender hex values. ## What changed - Updated DAI permit helper logic to mirror signer semantics: - Unlimited if `Boolean(allowed) === true` - Revoke if `Boolean(allowed) === false` or permit `value` coerces to `0` - Updated typed-sign date parsing to use `BigInt(value)` semantics for hex/decimal parity. - For out-of-range date values, UI now renders the raw value instead of showing a misleading timestamp. - Added regression tests for DAI permit coercion and typed-sign date parsing/rendering. ## Specific coercion case that triggers revoke A non-boolean `allowed` that coerces to `false` now correctly triggers revoke behavior in the UI, matching signing behavior. Example: - `allowed: ''` (empty string) -> `Boolean('') === false` -> revoke (Contrast: - `allowed: 'false'` -> `Boolean('false') === true`, so it is no longer shown as revoke.) ## Tests - `yarn jest --watchman=false app/components/Views/confirmations/utils/signature.test.ts` - `yarn jest --watchman=false app/components/Views/confirmations/components/data-tree/data-tree.test.tsx app/components/Views/confirmations/components/info/typed-sign-v3v4/simulation/components/value-display/value-display.test.tsx app/components/Views/confirmations/components/title/title.test.tsx` ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: ## **Related issues** Fixes: MetaMask/MetaMask-planning#6980 ## **Manual testing steps** Use this test dApp: https://web3.internal-five.ai-preview.cantina.sh/ ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile Coding Standards](https://github.com/MetaMask/metamask-mobile/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-mobile/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. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Touches permit revoke/unlimited classification used across confirmation title and simulation displays; incorrect coercion could mislabel approvals vs revokes, but changes are narrowly scoped and covered by new tests. > > **Overview** > Typed-sign confirmation logic now mirrors signer coercion for legacy DAI Permit: `allowed` is interpreted via JS truthiness (so string `'false'` is treated as *approve/unlimited*, while revoke is driven by falsy `allowed` or a zero-ish `value`). > > Date-like typed-sign fields (e.g., `expiry`) are now parsed via `BigInt` to correctly support hex inputs, and the UI falls back to displaying the raw string when values exceed JS safe integer range instead of showing an incorrect timestamp. Adds regression tests covering hex date parsing/out-of-range fallback and DAI permit/title/simulation behavior when `allowed` is `'false'`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5480331. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
1 parent 92c7175 commit f8392ec

File tree

6 files changed

+233
-12
lines changed

6 files changed

+233
-12
lines changed

app/components/Views/confirmations/components/data-tree/data-field.tsx

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,22 @@ function isTokenValueField(label: string, primaryType?: PrimaryType) {
6262
);
6363
}
6464

65+
function parseTypedSignDateValue(value: string) {
66+
try {
67+
const parsedValue = BigInt(value);
68+
if (
69+
parsedValue > BigInt(Number.MAX_SAFE_INTEGER) ||
70+
parsedValue < BigInt(Number.MIN_SAFE_INTEGER)
71+
) {
72+
return undefined;
73+
}
74+
75+
return Number(parsedValue);
76+
} catch {
77+
return undefined;
78+
}
79+
}
80+
6581
const createStyles = (depth: number) =>
6682
StyleSheet.create({
6783
container: {
@@ -99,13 +115,15 @@ const DataField = memo(
99115
if (type === 'address' && isValidHexAddress(value as Hex)) {
100116
fieldDisplay = <Address address={value} chainId={chainId} />;
101117
} else if (isDateField(label, primaryType) && Boolean(value)) {
102-
const intValue = parseInt(value, 10);
118+
const intValue = parseTypedSignDateValue(value);
103119

104120
fieldDisplay =
105121
intValue === NONE_DATE_VALUE ? (
106122
<Text>{strings('confirm.none')}</Text>
123+
) : intValue === undefined ? (
124+
<Text>{value}</Text>
107125
) : (
108-
<InfoDate unixTimestamp={parseInt(value, 10)} />
126+
<InfoDate unixTimestamp={intValue} />
109127
);
110128
} else if (isTokenValueField(label, primaryType)) {
111129
fieldDisplay = (

app/components/Views/confirmations/components/data-tree/data-tree.test.tsx

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import React from 'react';
33
import renderWithProvider from '../../../../../util/test/renderWithProvider';
44
import { backgroundState } from '../../../../../util/test/initial-root-state';
55
import DataTree, { DataTreeInput } from './data-tree';
6-
import { PrimaryTypeOrder } from '../../constants/signatures';
6+
import { PrimaryType, PrimaryTypeOrder } from '../../constants/signatures';
77
import { NONE_DATE_VALUE } from '../../utils/date';
88

99
const timestamp = 1647359825; // March 15, 2022 15:57:05 UTC
@@ -99,4 +99,61 @@ describe('NoChangeSimulation', () => {
9999
expect(getByText('Buy Amount')).toBeDefined();
100100
expect(getByText('100')).toBeDefined();
101101
});
102+
103+
it('parses hex date fields using bigint coercion', async () => {
104+
const { getByText } = renderWithProvider(
105+
<DataTree
106+
data={
107+
{
108+
expiry: {
109+
type: 'uint256',
110+
value: '0x62f5f80d',
111+
},
112+
} as DataTreeInput
113+
}
114+
chainId="0x1"
115+
primaryType={PrimaryType.Permit}
116+
/>,
117+
{
118+
state: {
119+
engine: {
120+
backgroundState,
121+
},
122+
},
123+
},
124+
);
125+
126+
expect(getByText('Expiry')).toBeDefined();
127+
expect(getByText('12 August 2022, 06:49')).toBeDefined();
128+
});
129+
130+
it('falls back to the original string for large hex date values', async () => {
131+
const largeHexDate =
132+
'0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff';
133+
134+
const { getByText } = renderWithProvider(
135+
<DataTree
136+
data={
137+
{
138+
expiry: {
139+
type: 'uint256',
140+
value: largeHexDate,
141+
},
142+
} as DataTreeInput
143+
}
144+
chainId="0x1"
145+
primaryType={PrimaryType.Permit}
146+
/>,
147+
{
148+
state: {
149+
engine: {
150+
backgroundState,
151+
},
152+
},
153+
},
154+
);
155+
156+
expect(getByText('Expiry')).toBeDefined();
157+
expect(getByText(largeHexDate)).toBeDefined();
158+
});
102159
});

app/components/Views/confirmations/components/info/typed-sign-v3v4/simulation/components/value-display/value-display.test.tsx

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,29 @@ describe('SimulationValueDisplay', () => {
238238
expect(await findByText('Unlimited')).toBeDefined();
239239
});
240240

241+
it('renders Dai Approve for allowed string "false" to match signing coercion', async () => {
242+
(
243+
useGetTokenStandardAndDetails as jest.MockedFn<
244+
typeof useGetTokenStandardAndDetails
245+
>
246+
).mockReturnValue(mockErc20TokenDetails);
247+
248+
const { findByText } = renderWithProvider(
249+
<SimulationValueDisplay
250+
canDisplayValueAsUnlimited
251+
modalHeaderText={'Spending cap'}
252+
tokenContract={'0x6b175474e89094c44da98b954eedeac495271d0f'}
253+
value={undefined}
254+
chainId={'0x1'}
255+
allowed={'false'}
256+
/>,
257+
{ state: mockInitialState },
258+
);
259+
260+
expect(await findByText('0x6B175...71d0F')).toBeDefined();
261+
expect(await findByText('Unlimited')).toBeDefined();
262+
});
263+
241264
it('invokes method to track missing decimal information for ERC20 tokens only once', async () => {
242265
(
243266
useGetTokenStandardAndDetails as jest.MockedFn<

app/components/Views/confirmations/components/title/title.test.tsx

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,40 @@ import Title from './title';
2929
jest.mock('../../hooks/useGetTokenStandardAndDetails');
3030

3131
describe('Confirm Title', () => {
32+
const typedSignRequestId = 'fb2029e1-b0ab-11ef-9227-05a11087c334';
33+
const daiPermitAllowedStringFalseData = JSON.stringify({
34+
types: {
35+
EIP712Domain: [
36+
{ name: 'name', type: 'string' },
37+
{ name: 'version', type: 'string' },
38+
{ name: 'chainId', type: 'uint256' },
39+
{ name: 'verifyingContract', type: 'address' },
40+
],
41+
Permit: [
42+
{ name: 'holder', type: 'address' },
43+
{ name: 'spender', type: 'address' },
44+
{ name: 'nonce', type: 'uint256' },
45+
{ name: 'expiry', type: 'uint256' },
46+
{ name: 'allowed', type: 'bool' },
47+
],
48+
},
49+
primaryType: 'Permit',
50+
domain: {
51+
name: 'Dai Stablecoin',
52+
version: '1',
53+
chainId: 1,
54+
verifyingContract: '0x6b175474e89094c44da98b954eedeac495271d0f',
55+
},
56+
message: {
57+
holder: '0x935e73edb9ff52e23bac7f7e043a1ecd06d05477',
58+
spender: '0x5B38Da6a701c568545dCfcB03FcB875f56beddC4',
59+
nonce: '0',
60+
expiry:
61+
'0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff',
62+
allowed: 'false',
63+
},
64+
});
65+
3266
const mockUseGetTokenStandardAndDetails = jest.mocked(
3367
useGetTokenStandardAndDetails,
3468
);
@@ -54,6 +88,43 @@ describe('Confirm Title', () => {
5488
).toBeTruthy();
5589
});
5690

91+
it('renders permit title for DAI permit when allowed is string "false"', () => {
92+
const state = merge({}, typedSignV4ConfirmationState, {
93+
engine: {
94+
backgroundState: {
95+
ApprovalController: {
96+
pendingApprovals: {
97+
[typedSignRequestId]: {
98+
requestData: {
99+
data: daiPermitAllowedStringFalseData,
100+
},
101+
},
102+
},
103+
},
104+
SignatureController: {
105+
signatureRequests: {
106+
[typedSignRequestId]: {
107+
messageParams: {
108+
data: daiPermitAllowedStringFalseData,
109+
},
110+
},
111+
},
112+
},
113+
},
114+
},
115+
});
116+
117+
const { getByText, queryByText } = renderWithProvider(<Title />, {
118+
state,
119+
});
120+
121+
expect(getByText('Spending cap request')).toBeTruthy();
122+
expect(
123+
getByText('This site wants permission to spend your tokens.'),
124+
).toBeTruthy();
125+
expect(queryByText('Remove permission')).toBeNull();
126+
});
127+
57128
it('renders the title and subtitle for a permit NFT signature', () => {
58129
const { getByText } = renderWithProvider(<Title />, {
59130
state: typedSignV4NFTConfirmationState,

app/components/Views/confirmations/utils/signature.test.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
11
import {
2+
isPermitDaiRevoke,
3+
isPermitDaiUnlimited,
24
parseAndNormalizeSignTypedData,
35
isRecognizedPermit,
46
isTypedSignV3V4Request,
@@ -15,6 +17,7 @@ import {
1517
SignatureRequest,
1618
SignatureRequestType,
1719
} from '@metamask/signature-controller';
20+
import { TOKEN_ADDRESS } from '../constants/tokens';
1821
import {
1922
mockTypedSignV3Message,
2023
personalSignSignatureRequest,
@@ -176,6 +179,27 @@ describe('Signature Utils', () => {
176179
});
177180
});
178181

182+
describe('DAI permit coercion helpers', () => {
183+
it('mirrors bool truthiness coercion for DAI permit unlimited checks', () => {
184+
expect(isPermitDaiUnlimited(TOKEN_ADDRESS.DAI, true)).toBe(true);
185+
expect(isPermitDaiUnlimited(TOKEN_ADDRESS.DAI, 'false')).toBe(true);
186+
expect(isPermitDaiUnlimited(TOKEN_ADDRESS.DAI, 0)).toBe(false);
187+
});
188+
189+
it('does not classify DAI permit as revoke when allowed is the string "false"', () => {
190+
expect(isPermitDaiRevoke(TOKEN_ADDRESS.DAI, 'false')).toBe(false);
191+
});
192+
193+
it('classifies DAI permit as revoke for zero-like numeric values', () => {
194+
expect(isPermitDaiRevoke(TOKEN_ADDRESS.DAI, undefined, '0x0')).toBe(true);
195+
expect(isPermitDaiRevoke(TOKEN_ADDRESS.DAI, undefined, '0')).toBe(true);
196+
});
197+
198+
it('classifies DAI permit as revoke when allowed is an empty string', () => {
199+
expect(isPermitDaiRevoke(TOKEN_ADDRESS.DAI, '')).toBe(true);
200+
});
201+
});
202+
179203
describe('isRecognizedPermit', () => {
180204
it('returns true for recognized permit types', () => {
181205
const mockRequest: SignatureRequest = {

app/components/Views/confirmations/utils/signature.ts

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,32 +40,60 @@ interface ValueType {
4040
/**
4141
* Support backwards compatibility DAI while it's still being deprecated. See EIP-2612 for more info.
4242
*/
43+
const coerceAllowedToBoolean = (
44+
allowed?: number | string | boolean | null,
45+
): boolean | undefined => {
46+
if (allowed === undefined) {
47+
return undefined;
48+
}
49+
50+
return Boolean(allowed);
51+
};
52+
53+
const coerceNumberishToBigInt = (
54+
value?: number | string | BigNumber | null,
55+
): bigint | undefined => {
56+
if (value === undefined || value === null) {
57+
return undefined;
58+
}
59+
60+
try {
61+
if (value instanceof BigNumber) {
62+
return BigInt(value.toFixed());
63+
}
64+
return BigInt(value);
65+
} catch {
66+
return undefined;
67+
}
68+
};
69+
4370
export const isPermitDaiUnlimited = (
4471
tokenAddress: string,
45-
allowed?: number | string | boolean,
72+
allowed?: number | string | boolean | null,
4673
) => {
4774
if (!tokenAddress) return false;
4875

76+
const parsedAllowed = coerceAllowedToBoolean(allowed);
77+
4978
return (
5079
tokenAddress.toLowerCase() === TOKEN_ADDRESS.DAI.toLowerCase() &&
51-
Number(allowed) > 0
80+
parsedAllowed === true
5281
);
5382
};
5483

5584
export const isPermitDaiRevoke = (
5685
tokenAddress: string,
57-
allowed?: number | string | boolean,
58-
value?: number | string | BigNumber,
86+
allowed?: number | string | boolean | null,
87+
value?: number | string | BigNumber | null,
5988
) => {
6089
if (!tokenAddress) return false;
6190

91+
const parsedAllowed = coerceAllowedToBoolean(allowed);
92+
const parsedValue = coerceNumberishToBigInt(value);
93+
6294
return (
6395
tokenAddress.toLowerCase() === TOKEN_ADDRESS.DAI.toLowerCase() &&
64-
(allowed === 0 ||
65-
allowed === false ||
66-
allowed === 'false' ||
67-
value === '0' ||
68-
(value instanceof BigNumber && value.eq(0)))
96+
(parsedAllowed === false || parsedValue === BigInt(0))
6997
);
7098
};
7199

0 commit comments

Comments
 (0)