-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Comments
@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" |
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. :) |
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
I see the message is coming from here, but not sure why I can't have more than one parent type ethers.js/packages/hash/src.ts/typed-data.ts Line 214 in 8614665
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
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...."
} |
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. |
@ricmoo exciting to have this directly inside of ethers v5. How come the |
The underscore is just temporary until people are happy with the API. Then it will be renamed. :) |
Neat, thats a cool way to introduce experimental/new features to prod dists |
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. |
Great work on this! I'm trying to use
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 |
@markdreyer Thanks for trying it out. I'll investigate first thing tomorrow... |
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? |
@Mark-UNCX Can you create a brand new private key (and remove it afterward) for the mobile version and include the following for
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 Also, this probably makes more sense to go into a new issue, so this issue can continue tracking the EIP-712 implementation. :) |
Working as expected now - thanks @ricmoo |
I dont understand, should I remove EIP712Domain: from types and use ^5.0.24 |
@livingrock7 yupp. :) |
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 |
I am encountering an error when using linked issue here: NomicFoundation/hardhat#1199 |
_signTypedData method on ethers does not match with ERC712 solidity code
in the SC the recovered signer is : |
Found my problem |
I want to confirm that in order to |
@EvilJordan That is correct. It uses EIP-191 as best I know. :) |
This issue is complete and I won't be changing the API in v5, but in v6 the underscore ( Thanks! :) |
@ricmoo does 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. |
The The JsonRpcSigner uses explicitly specified |
i am also having issue in calling the signTypedData can you help me out it says it is not a function ? |
@danirabbani90 In v5 it is |
emm... may I ask what to do with the signature? |
Gives "ambiguous primary types or unused types" error ethers-io/ethers.js#687 (comment)
@ricmoo, I need your help |
I feel this should be more clear on the documentation. |
You SHOULD NOT include Wasted a day to figure this out. |
Exactly. @ricmoo I wish you fix this in the next stable release. |
Hi! I'm testing signing messages using the metamask's example app: https://metamask.github.io/test-dapp/ For
The
I am removing the What's the best solution here? Should the message be considered invalid or should ethers ignore unused types instead of crashing? |
@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. :) |
Thank you for clearing that up!!! It worked after removing EIP712Domain from types
|
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. :)
The text was updated successfully, but these errors were encountered: