Add EIP 712 helpers

[frangio]

This adds a contract that implements an efficient calculation of the EIP712 domain separator.

The code I used to measure gas costs is in frangio/domain-separator-gas-report.

The constructor looks ugly but I think it's the price of efficient code!

[spalladino]

Awesome implementation @frangio! I have two main things I want to discuss:

• Salt: I don't think I've ever seen a usage of salt in the wild, and the EIP is not very descriptive about it, but if a project used it, I'd expect it to store it instead of the saltless one we're storing today in the constructor. Perhaps we should allow the user to set an optional salt in the constructor directly? Another thing worth considering: if a contract is using a salt to disambiguate, it means they may have more than a single domain separator. In that case, it makes sense not to have a distinguished _CACHED_DOMAIN_SEPARATOR over another. Bottom line: do you think it would be a good idea to provide EIP712 as an internal library with just the buildDomainSeparator function, which users can rely upon when they have more than a single separator (or other use case), and then have this base contract use it?

• Digest: Looking at the permit code in dai, it'd seem there is something we can abstract. Perhaps we can add an internal digest() function to the contract that does something like the following?

function digest(dataHash) {
  return keccak256(abi.encodePacked("\x19\x01", getDomainSeparator(), dataHash));
}

[frangio]

Salt

I actually designed the API assuming the salt would be different for every signature that gets verified. But now that you mention it I think I was mistaken.

if a contract is using a salt to disambiguate, it means they may have more than a single domain separator.

I understand how you reached this conclusion but I find it hard to imagine in what situation this would be used. I looked through the EIP discussions and couldn't find anything. If a contract can receive different types of signed messages they will be different because of the message encoding so a salt isn't necessary.

I'm tempted to drop the part about the salt since we don't really understand it and it doesn't seem to be in use.

As for exposing buildDomainSeparator (note that it's currently private), as it is now I don't think it's a good idea because it relies on passing in the right typeHash argument. I prefer to just ship _domainSeparator() with proper caching + built in _digest. If someone needs something different we'll find out!

Digest

Yeah, I think this is a good idea, specially because the dataHash argument would be a bytes32 on the stack so I don't think there will be significant overhead (compared to an argument on memory like a bytes array).

This one also makes me think we should drop the salt part, so that we can unambiguously use the domain separator without a salt.

[spalladino]

Agree with both!

[frangio]

I added a _digest function but I'm not happy with the name. I accept suggestions, I'll think about it later myself too.

[mverzilli]

I added a _digest function but I'm not happy with the name. I accept suggestions, I'll think about it later myself too.

Curious about this, why don't you like the name? Is it because it lacks specificity? If so, maybe borrowing jargon from https://eips.ethereum.org/EIPS/eip-712#specification we could call it _structuredDataDigest or _encodeStructuredData?

[spalladino]

+1 to _structuredDataDigest. An alternative would be something like toStructuredDataHash, following the same structure as ECDSA#toEthSignedMessageHash.

[frangio]

Okay, I didn't mean to ignore your suggestions (I almost went with _structuredDataDigest) but I decided for a name based on the signTypedData JSON RPC call (which is what developers will be interacting with), and went with _hashTypedData.

[frangio]

Or... maybe it's better to go with _hashTypedDataV4 since the RPC method is actually signTypedData_v4. Reference.

[frangio]

I've moved the contract to a new drafts directory. I will merge once you confirm the new directory and description looks good to you.

[spalladino]

Looks great @frangio! I think it's good to merge. But there are two things worth looking into after this:

1- Expanding on the usage of this EIP. Though you already briefly covered it in the reference of the _hashTypedDataV4 method, I think it's worth expanding a bit more on encodeData and provide a friendlier description than that of the EIP.

2- Discoverability. Unless you already know that EIP712 is included as a Draft, it's impossible to find it in the docs. I'd suggest adding a mention to it from the Cryptography section at the very least, and consider other solutions (such as finally implementing search, or adding just a plain list of all the contracts in the repo so a reader can easily ctrl+f through it).

[spalladino]

adding a mention to it from the Cryptography section at the very least,

Actually, from looking at the PR, it seems like it's added in both Drafts and Cryptography, but in the deploy preview it's only listed in Drafts. Not sure what's happening there.

[frangio]

I left the line in the cryptography readme accidentally. It won't show up in the site because of how solidity-docgen works. (When used in the readme mode, it only makes available in a readme the contracts that are under the same directory.)

Placing a link in Cryptography is a great idea though, and having an index page that is a list of all contracts (and maybe even functions) is also a great idea! Adding search might be a similar level of effort though. 😅