Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Missing "Unlimited" as value for the DAI permit #29597

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

pedronfigueiredo
Copy link
Contributor

@pedronfigueiredo pedronfigueiredo commented Jan 9, 2025

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'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.

Open in GitHub Codespaces

Related issues

Fixes: #28964

Manual testing steps

  1. Open the browser console and execute the following code:
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();
  1. Change "allowed": true to "allowed": false in the aforementioned code and execute it to see the revocation screen.

Screenshots/Recordings

Before

After

Screenshot 2025-01-08 at 18 30 35 Screenshot 2025-01-08 at 18 30 37 Screenshot 2025-01-09 at 10 22 09 Screenshot 2025-01-09 at 10 22 13

Pre-merge author checklist

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.

@pedronfigueiredo pedronfigueiredo added the team-confirmations Push issues to confirmations team label Jan 9, 2025
@pedronfigueiredo pedronfigueiredo self-assigned this Jan 9, 2025
@pedronfigueiredo pedronfigueiredo requested a review from a team as a code owner January 9, 2025 10:39
Copy link
Contributor

github-actions bot commented Jan 9, 2025

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@metamaskbot
Copy link
Collaborator

Builds ready [715eb4f]
Page Load Metrics (1654 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1441186416559847
domContentLoaded13921797162910149
load1441185816549747
domInteractive22132473215
backgroundConnect1183282010
firstReactRender1595402613
getState512821
initialActions01000
loadScripts1037140912289244
setupStore684202311
uiStartup162726521915212102

@pedronfigueiredo pedronfigueiredo requested review from a team as code owners January 9, 2025 11:26
@metamaskbot
Copy link
Collaborator

Builds ready [6c106c2]
Page Load Metrics (1646 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint20821121507450216
domContentLoaded14892076162716680
load15002132164617484
domInteractive24176564521
backgroundConnect76623199
firstReactRender1597373014
getState57615199
initialActions01000
loadScripts10501618120714570
setupStore690282914
uiStartup165224931994241116

@metamaskbot
Copy link
Collaborator

Builds ready [c2b7ffb]
Page Load Metrics (1760 ± 77 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15442271175615775
domContentLoaded15282259171415775
load15432274176016177
domInteractive2586472210
backgroundConnect996372813
firstReactRender17102453015
getState594232612
initialActions01000
loadScripts11501794128113464
setupStore690222613
uiStartup182327652114251121

@metamaskbot
Copy link
Collaborator

Builds ready [fb248dc]
Page Load Metrics (1693 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint32518261623306147
domContentLoaded1558179616656933
load1572182716937536
domInteractive259438189
backgroundConnect95726168
firstReactRender1696392813
getState57120209
initialActions01000
loadScripts1154137012496330
setupStore76717189
uiStartup17802404197114771

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-confirmations Push issues to confirmations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Missing "Unlimited" as value for the DAI permit
2 participants