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

Add EIP-712 Ancillary Package for v5. #687

Closed
ricmoo opened this issue Dec 16, 2019 · 41 comments
Closed

Add EIP-712 Ancillary Package for v5. #687

ricmoo opened this issue Dec 16, 2019 · 41 comments
Labels
bug Verified to be an issue. enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@ricmoo
Copy link
Member

ricmoo commented Dec 16, 2019

This is a feature a few people have requested.

While I'm not a fan of EIP-712 it is a feature that some people are using, so will need to be supported.

With the upcoming release of v5, this will be easier to add as an ancillary package, which means that people who require it will have access to it, but people without the need for it won't be required to pull in the overly complicated and non-trivial package. :)

This is just a short GitHub Issue to help track progress and let people know it is on the roadmap. :)

@pkieltyka
Copy link

pkieltyka commented Aug 6, 2020

@ricmoo just FYI, I've implemented this in another package using ethers v5

https://github.com/arcadeum/ethers-eip712

it's quite succinct and works well. feel free to copy/paste it in, or I can submit a PR .. lmk which package its best suited, but perhaps a new one called "typed-data"

@ricmoo
Copy link
Member Author

ricmoo commented Oct 19, 2020

I've added this functionality to ethers, please try it out. :)

@pkieltyka I've used MetaMasks library to generate fuzz tests to validate my implementation against, but will look into add yours too, to validate theirs. :)

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Oct 19, 2020
@pi0neerpat
Copy link

Awesome! I'm testing this out by converting an existing web3.js typed data message to use this. Having a bit of trouble understanding this error ambiguous primary types or unused types

ethers 5.0.18

I see the message is coming from here, but not sure why I can't have more than one parent type

logger.throwArgumentError(`ambiguous primary types or unused types: ${ primaryTypes.map((t) => (JSON.stringify(t))).join(", ") }`, "types", types);

My code and logs

    const signer = await walletProvider.getSigner();
    console.log(domain);
    console.log(types);
    console.log(value);
    const signature = await signer._signTypedData(domain, types, value);

domain

{
  "name": "kChannels MVP",
  "version": 1,
  "chainId": 1,
  "verifyingContract": "0x1944517aA5c7D02731A7efAa1023d5C998996461",
  "salt": "0x8a102c4640cfefb2eda4ee058d8783d669df911a16140681aeae27bfcb6c93a3"
}

types

{
  "EIP712Domain": [
    {
      "name": "name",
      "type": "string"
    },
    {
      "name": "version",
      "type": "uint256"
    },
    {
      "name": "chainId",
      "type": "uint256"
    },
    {
      "name": "verifyingContract",
      "type": "address"
    },
    {
      "name": "salt",
      "type": "bytes32"
    }
  ],
  "AuthenticationChallenge": [
    {
      "name": "text",
      "type": "string"
    },
    {
      "name": "client_unpredictable_number",
      "type": "uint256"
    },
    {
      "name": "unpredictable_number",
      "type": "uint256"
    },
    {
      "name": "client_ip",
      "type": "string"
    },
    {
      "name": "issued_at",
      "type": "uint256"
    },
    {
      "name": "expires_at",
      "type": "uint256"
    },
    {
      "name": "issuer_signature",
      "type": "string"
    },
    {
      "name": "signing_identity",
      "type": "address"
    }
  ]
}

value (some values redacted)

{
  "text": "Welcome to Kchannels MVP! Please sign this message to authenticate.",
  "client_unpredictable_number": "33215658837458338000",
  "unpredictable_number": "5213154976",
  "client_ip": "REDACTED",
  "issued_at": "1603321565",
  "expires_at": "1603322165",
  "signing_identity": "REDACTED",
  "issuer_signature": "0x9605a5731e...."
}

@ricmoo
Copy link
Member Author

ricmoo commented Oct 22, 2020

You should not pass the EIP712Domain into ethers. It will compute it for you. It is seeing that as an alternate possible root.

You can’t have more than one root parent since that would mean you are passing in a bunch of unused types. You can pass in nested structures, just not cyclic ones...

I am considering letting the EIP712Domain and if it is present, ignore it as a ca dosage if there are two roots. But that is a bunch of extra validation that will be required too. It’s on my short list of possible changes.

@pkieltyka
Copy link

@ricmoo exciting to have this directly inside of ethers v5. How come the signTypedData method is underscored in the prefix? ie. https://github.com/ethers-io/ethers.js/blob/master/packages/tests/src.ts/test-utils.ts#L773 as well https://github.com/ethers-io/ethers.js/blob/master/packages/tests/src.ts/test-utils.ts#L759

@ricmoo
Copy link
Member Author

ricmoo commented Oct 22, 2020

The underscore is just temporary until people are happy with the API. Then it will be renamed. :)

@pkieltyka
Copy link

Neat, thats a cool way to introduce experimental/new features to prod dists

@pi0neerpat
Copy link

EIP712Domain into ethers. It will compute it for you

That did the trick!

Now the server (some else's) is rejecting the signature/message pair. Will report back if I get it fully working.

@markdreyer
Copy link

Great work on this! I'm trying to use verifyTypedData to verify the signer address, but it returns a different address than what it was signed with. Am I calling it wrong or is there something with the implementation that changed?

ethers v5.0.23
MetaMask v8.1.6 (chrome extension)
const provider = new ethers.providers.Web3Provider(window.ethereum)
const signer = provider.getSigner()
const domain = {
    name: 'My Messaging App',
    version: '1',
    chainId: 5,
    verifyingContract: '0x7753cfAD258eFbC52A9A1452e42fFbce9bE486cb'
};

const types = {
    Message: [
        { name: 'content', type: 'string' }
    ]
};

const message = {
    content: 'a signed message'
};
signer.getAddress().then(walletAddress => {
    signer._signTypedData(domain, types, message)
        .then(signature => {
            let verifiedAddress = ethers.utils.verifyTypedData(domain, types, message, signature)
            if (verifiedAddress !== walletAddress) {
                alert(`Signed by: ${verifiedAddress}\r\nExpected: ${walletAddress}`)
            }
        })
})
// DEBUG
// walletAddress    = 0x5d2D4CcEDdb0C49972A18D6dD18FAb4cAA3a2F31
// signature        = 0x0b4287a61819d1a4198a3db329eaeed2dcab7d5d6e063d346d348687156ecb8813f23dbc0e1ad241be78b3b0d2f330b3315205217f14de9160c1dd94d8a9c7bc1c
//  verifiedAddress = 0x4A2064A9a3732b651FB27D47b8503a4B2B35f065

@ricmoo
Copy link
Member Author

ricmoo commented Dec 8, 2020

@markdreyer Thanks for trying it out. I'll investigate first thing tomorrow...

@James-Unicrypt
Copy link

Hey guys, Im having no luck using signMessage() on mobile wallets such as metamask or trust wallet, it verifies to the incorrect address. Really struggling to get ._signTypedData to work on mobile wallets too. Can anyone provide a decent example? I can confirm signMessage is working on desktop through metamask. Problem is mobile wallets. Anyone else struggling with this?

@ricmoo
Copy link
Member Author

ricmoo commented Dec 9, 2020

@Mark-UNCX Can you create a brand new private key (and remove it afterward) for the mobile version and include the following for signMessage (create the simplest possible code to demonstrate the issue):

  • private key
  • the message
  • the signature
  • the address that is being recovered (incorrectly)
  • your code

I don't know what mobile could be doing differently, but that is at least some place I can start to try diagnosing the problem. I've not had any issues with signMessage on iOS (Safari), so please also include the version of the OS and what platforms (Chrome, Safari, etc.)

Also, this probably makes more sense to go into a new issue, so this issue can continue tracking the EIP-712 implementation. :)

@jamesmorgan
Copy link

Working as expected now - thanks @ricmoo

@livingrock7
Copy link

I dont understand, should I remove EIP712Domain: from types and use ^5.0.24
Any help is appreciated thanks!

@ricmoo
Copy link
Member Author

ricmoo commented Dec 26, 2020

@livingrock7 yupp. :)

@pkieltyka
Copy link

pkieltyka commented Jan 13, 2021

I've been using the _signTypedData method in ethers and looking good so far! one tiny thing I noticed is the typedData object param is being double serialized when sending over the wire -- you can drop the JSON.stringify here https://github.com/ethers-io/ethers.js/blob/master/packages/providers/src.ts/json-rpc-provider.ts#L234

@thegostep
Copy link

I am encountering an error when using _signTypedData with hardhat.

linked issue here: NomicFoundation/hardhat#1199

@zgorizzo69
Copy link

zgorizzo69 commented Sep 10, 2021

_signTypedData method on ethers does not match with ERC712 solidity code
I am using ethers 5.4.6, openzeppelin/contracts 4.3.1 and hardhat ethers 2.0.2

const MinimalForwarderContractFactory = await ethers.getContractFactory(
    'MinimalForwarder',
  ); // comes from @openzeppelin/contracts/metatx/MinimalForwarder.sol
  const forwarder = await MinimalForwarderContractFactory.deploy();
  await forwarder.deployed();
 let { chainId } = await ethers.provider.getNetwork();
 domain = {
      name: 'Testing',
      version: '0.0.1',
      chainId,
      verifyingContract: forwarder.address,
    };
const Transaction = [
  { name: 'from', type: 'address' },
  { name: 'to', type: 'address' },
  { name: 'value', type: 'uint256' },
  { name: 'gas', type: 'uint256' },
  { name: 'nonce', type: 'uint256' },
  { name: 'data', type: 'bytes' },
];
 
const types = { Transaction };

 let gasLimit = await ethers.provider.estimateGas({
      to: homeFiContract.address,
      from: signers[0].address,
      data: data,
    });

    let message = {
      from: signers[0].address,
      to: homeFiContract.address,
      value: 0,
      gas: gasLimit.toNumber(),
      nonce: 0,
      data: '0x',
    };
 
    const signature = await signers[0]._signTypedData(domain, types, message);
    const verifiedAddress = ethers.utils.verifyTypedData(
      domain,
      types,
      message,
      signature,
    );
      expect(verifiedAddress).to.equal(signers[0].address); // works !
       const verifiedFromContract = await forwarder.verify(message, signature);
       expect(verifiedFromContract).to.equal(true); // doesn't work !

in the SC the recovered signer is :0x29501274044eb125ef4d5605b5c25630e722dd44 instead of req.from:0xf39fd6e51aad88f6f4ce6ab8827279cfffb92266 which is indeed the passed hardhat signer 0
Do you have an idea why ?

@zgorizzo69
Copy link

Found my problem
For info Inside minimalForwarder.sol _TYPEHASH = keccak256("ForwardRequest(
so the type Transaction must be renamed to ForwardRequest

@EvilJordan
Copy link

EvilJordan commented Oct 16, 2021

I want to confirm that in order to verify an EIP-191 signedMessage, to be compatible with the emerging Sign-In with Ethereum standard, ethers.utils.verifyMessage is my friend. What say you, Ricketh of Moos?

@ricmoo
Copy link
Member Author

ricmoo commented Oct 16, 2021

@EvilJordan That is correct. It uses EIP-191 as best I know. :)

@ricmoo
Copy link
Member Author

ricmoo commented Oct 20, 2021

This issue is complete and I won't be changing the API in v5, but in v6 the underscore (_) will be dropped, so I'm going to close it now.

Thanks! :)

@ricmoo ricmoo closed this as completed Oct 20, 2021
@midgerate
Copy link

midgerate commented Nov 3, 2021

@ricmoo does _signTypedData uses eth_signTypedData_v4 to create the signature?
I found that v4 signatures also require primaryType as part of data.

image

My message contains an array that is only supported in v4 and it seems that metamask mobile (with walletconnect) uses v3 by default, unless explicitly mentioned. It works fine on trust wallet.

@ricmoo
Copy link
Member Author

ricmoo commented Nov 3, 2021

The primaryType is automatically computed from the request. The ethers API only accepts non-computable parameters, so that validation is implicit.

The JsonRpcSigner uses explicitly specified _v4 though, so I would think it would work?

@danirabbani90
Copy link

i am also having issue in calling the signTypedData can you help me out it says it is not a function ?

@ricmoo
Copy link
Member Author

ricmoo commented Nov 11, 2021

@danirabbani90 In v5 it is ._signTypedData is the method.

@SiestaMadokaist
Copy link

emm... may I ask what to do with the signature?
how do I combine the signature and the transaction so I can submit it to the blockchain?

@kayceejenz
Copy link

@ricmoo, I need your help
I've been trying to verify a signature signed by another signer with "ethers.utils.verifyTypedData(domain, types, tokenTransferObj, signature);". But It returns a different signer address from the actual signer.
Please, what'd be the problem?

@0xdcota
Copy link

0xdcota commented Aug 5, 2022

You should not pass the EIP712Domain into ethers. It will compute it for you. It is seeing that as an alternate possible root.

You can’t have more than one root parent since that would mean you are passing in a bunch of unused types. You can pass in nested structures, just not cyclic ones...

I am considering letting the EIP712Domain and if it is present, ignore it as a ca dosage if there are two roots. But that is a bunch of extra validation that will be required too. It’s on my short list of possible changes.

I feel this should be more clear on the documentation.

@nftgeek
Copy link

nftgeek commented Aug 9, 2022

@ricmoo, I need your help I've been trying to verify a signature signed by another signer with "ethers.utils.verifyTypedData(domain, types, tokenTransferObj, signature);". But It returns a different signer address from the actual signer. Please, what'd be the problem?

You SHOULD NOT include EIP712Domain when signing using _signTypedData() of ethers.
You SHOULD include EIP712Domain when recovering using recoverTypedSignature of @metamask/eth-sig-util

Wasted a day to figure this out.

@nftgeek
Copy link

nftgeek commented Aug 9, 2022

You should not pass the EIP712Domain into ethers. It will compute it for you. It is seeing that as an alternate possible root.
You can’t have more than one root parent since that would mean you are passing in a bunch of unused types. You can pass in nested structures, just not cyclic ones...
I am considering letting the EIP712Domain and if it is present, ignore it as a ca dosage if there are two roots. But that is a bunch of extra validation that will be required too. It’s on my short list of possible changes.

I feel this should be more clear on the documentation.

Exactly. @ricmoo I wish you fix this in the next stable release.

@everdimension
Copy link

Hi! I'm testing signing messages using the metamask's example app: https://metamask.github.io/test-dapp/

For eth_signTypedData_v4 it sends an object that Ethers cannot sign:

{"domain":{"chainId":"137","name":"Ether Mail","verifyingContract":"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC","version":"1"},"message":{"contents":"Hello, Bob!","from":{"name":"Cow","wallets":["0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826","0xDeaDbeefdEAdbeefdEadbEEFdeadbeEFdEaDbeeF"]},"to":[{"name":"Bob","wallets":["0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB","0xB0BdaBea57B0BDABeA57b0bdABEA57b0BDabEa57","0xB0B0b0b0b0b0B000000000000000000000000000"]}]},"primaryType":"Mail","types":{"EIP712Domain":[{"name":"name","type":"string"},{"name":"version","type":"string"},{"name":"chainId","type":"uint256"},{"name":"verifyingContract","type":"address"}],"Group":[{"name":"name","type":"string"},{"name":"members","type":"Person[]"}],"Mail":[{"name":"from","type":"Person"},{"name":"to","type":"Person[]"},{"name":"contents","type":"string"}],"Person":[{"name":"name","type":"string"},{"name":"wallets","type":"address[]"}]}}

The types object here has the following fields:

  • EIP712Domain
  • Group
  • Mail
  • Person

I am removing the EIP712Domain as suggested in this topic before passing it to signer._signTypedData, but the Group and Mail are also unused and the method crashes with the _ ambiguous primary types or unused types_ error

What's the best solution here? Should the message be considered invalid or should ethers ignore unused types instead of crashing?

@ricmoo
Copy link
Member Author

ricmoo commented Aug 16, 2022

@everdimension You should remove unused types, it is invalid for ethers. It is not possible to ignore unused types, since ethers constructs the dependency graph to determine what the primaryType is; if there are unused types, there are multiple roots in the DAG, and therefore it is impossible to tell which is the primaryType. The ethers API aims to be minimal, which means not needing the same data specified multiple times.

It is bad form anyways to include random data that isn't used, especially when passion it off to be signed. :)

@0xkakashixyz
Copy link

0xkakashixyz commented Jan 28, 2023

@ricmoo, I need your help I've been trying to verify a signature signed by another signer with "ethers.utils.verifyTypedData(domain, types, tokenTransferObj, signature);". But It returns a different signer address from the actual signer. Please, what'd be the problem?

You SHOULD NOT include EIP712Domain when signing using _signTypedData() of ethers. You SHOULD include EIP712Domain when recovering using recoverTypedSignature of @metamask/eth-sig-util

Wasted a day to figure this out.

Thank you for clearing that up!!! It worked after removing EIP712Domain from types


interface ITypes { name: string, type: string }
interface IEIP712 { 
    types: {ForwardRequest: ITypes[]},
    domain: { name: string, version: string, chainId: number, verifyingContract: string},
    primaryType: string 
}
const ForwardRequest: ITypes[] = [
    { name: 'from', type: 'address' },
    { name: 'to', type: 'address' },
    { name: 'value', type: 'uint256' },
    { name: 'gas', type: 'uint256' },
    { name: 'nonce', type: 'uint256' },
    { name: 'data', type: 'bytes' },
  ];
function getMetaTxTypeData(chainId: number, verifyingContract: string): IEIP712 {
    return {
        types: {
            ForwardRequest,
        },
        domain: {
            name: 'RootForwarder',
            version: '0.0.2',
            chainId,
            verifyingContract
        },
        primaryType: 'ForwardRequest'
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified to be an issue. enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests