EIP-712: fix `eth_signTypedData` definition

[wchargin]

The previous EIP text for eth_signTypedData seemed to describe the existing "Ethereum signed message" flows, without any relation to typed data. The new definition is consistent with what's actually implemented by (e.g.) the Ethers signer and the OpenZeppelin validator, as well as with the start of the "Specification" section of this EIP.

wchargin-branch: eip-712-eth-signtypeddata

[eth-bot]

File EIPS/eip-712.md

Requires 1 more reviewers from @dekz, @logvinovleon, @recmo Requires 2 more reviewers from @axic, @gcolvin, @lightclient, @pandapip1, @samwilsn

[Pandapip1]

This makes a change to a final EIP. The implementations will need to be changed.

[wchargin]

The implementations will need to be changed.

I believe that this patch updates the text to match the implementations (and the intent), not the other way around. Am I misunderstanding?

I guess I don't really understand what it would mean for the eth_signTypedData RPC to be defined just like eth_sign and not actually use any of the typed data information.

In addition to Ethers and OpenZeppelin (as listed above), other prominent implementations that are consistent with this patch include OpenSea's Seaport, Uniswap V3's position NFTs, and MetaMask's signing libraries. Are there actually implementations that would need to be updated here?

This makes a change to a final EIP.

(For what it's worth, I also did raise this before the EIP was finalized, but received no response. Which is okay, no worries, but it seems better to fix late than never if the fix is indeed correct.)

[Pandapip1]

I believe that this patch updates the text to match the implementations

Yes, but that isn't allowed to a final EIP. I personally am okay with this change, but other editors may disagree.

[xinbenlv]

Thank you @wchargin for this update.

I was against the 712 to go final at the current snapshot. but since it is now Final, unless we make a EIP rule change, this definition change is non-compatible with the final version and hence I don't think it shall be approved. It seems the only way to make this improvement is to start a competing EIP that defines a different sign typed data.

[Pandapip1]

I feel like I should make a bot that adds a comment on every PR that moves an EIP to final that the entire EIP must be proofread. It seems obvious to me, but here we are...

[xinbenlv]

IMO pure editorial change that improves readability shall be allowed, but breaking change of the specificiation are not allowed for final. If an EIP intends to be a standard/protocol/interface, the behaviors mandated by spec shall not change after becoming FINAL.

For example, if the original spec is

interface {
  function transfer(uint256 id);
}

This change shall be allowed after an EIP becomes FINAL:

interface {
  function transfer(uint256 tokenId);
}

This change IMO shall NOT be allowed after an EIP becomes FINAL:

interface {
  function transferToken(uint256 id);
}

This is because the former is compatible with original. The latter is not.

In this particular PR addressing EIP-712, it is updating the msg digest being hashed, can and will change signature. Therefore it's not compatible with the FINAL version of 712. Unfortunately IMO it shall not be allowed for updating 712 spec in this way.

[teamdandelion]

My understanding is that as written, the EIP is literally incorrect and includes inaccurate information, and that this pull request fixes that, so that the EIP will correctly describe the real behavior of all compliant implementations. Can we agree that the top priority should be that the EIP is an accurate reference?

Maybe original authors (@recmo @LogvinovLeon @dekz) can comment?

[xinbenlv]

It is not uncommon that early adoptors of EIP drafts have been using outdated behaviors or made their local design choices.

Let's let original authors respond. I think the more accurate way to say is current 712 snapshot has not yet eliminated all ambiguity and ethers and open-zeppelin have adopted non-standard approach.

[Pandapip1]

I guess one could also do the "SHOULD" and "SHOULD NOT" trick to correct EIPs.

- MUST do X
+ SHOULD do Y
+ SHOULD NOT do X

Technically this isn't backwards incompatible.

[xinbenlv]

@Pandapip1

I guess one could also do the "SHOULD" and "SHOULD NOT" trick to correct EIPs.

- MUST do X
+ SHOULD do Y
+ SHOULD NOT do X

Technically this isn't backwards incompatible.

I believe it would cause backward incompatiblility in some case.

Suppose the following change was proposal for the final EIP-X:

- MUST implement `iterate(uint)`
+ SHOULD implement `iterate(uint)`

A client assuming the smart contract is EIP-X final version will assume iterate(uint) interface to exist. But after the change, the iterate(uint) may not exist in certain case and therefore client would have to deal with such scenarios .

per https://www.rfc-editor.org/rfc/rfc2119

This word, or the adjective "RECOMMENDED", mean that there
   may exist valid reasons in particular circumstances to *ignore a
   particular item*, but the full implications must be understood and
   carefully weighed before choosing a different course.

[Pandapip1]

I guess I didn't think about that particular case. You are correct there.

As written, however, this doesn't make any sense since it references a nonexistent variable. Therefore, since there are no applications that can correctly parse this, there is no backward compatibility issue there since there is nothing that was previously compatible.

[xinbenlv]

I guess I didn't think about that particular case. You are correct there.

As written, however, this doesn't make any sense since it references a nonexistent variable. Therefore, since there are no applications that can correctly parse this, there is no backward compatibility issue there since there is nothing that was previously compatible.

I don't know if we are referring to the same aspect of this PR. I am referring to this part

- The sign method calculates an Ethereum specific signature with: `sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message)))`.
+ The sign method calculates an Ethereum specific signature with: `sign(keccak256("\x19\x01" + domainSeparator + structHash))`, as defined in the "Specification" section above.

The particular "\x19Ethereum Signed Message:\n" + len(message) was removed and replaced by domainSeparator. I think one could potentially implement the former version that has Ethereum Signed Message as prefix.

When you say "nonexistent variable", which nonexistent variable do you refer to?

[Pandapip1]

message is not defined.

[xinbenlv]

Here is what caused me to think message is defined:

In the spec section:

• https://github.com/ethereum/EIPs/blob/6fe3596850403b3cc1257522de24fa426c607027/EIPS/eip-712.md?plain=1#L82
• https://github.com/ethereum/EIPs/blob/6fe3596850403b3cc1257522de24fa426c607027/EIPS/eip-712.md?plain=1#L87 both have mentioned message, assuming to be the message being signed.

If the rationale for accepting this PR is due to message not defined, with same argument requires to update L82 and L87 too, I guess? I'd lean towards leaving the author to decide.

[Pandapip1]

I agree that the first line defines message. The second line doesn't. That's still a good enough indication that message is defined.

As @xinbenlv mentioned in discord, there appear to be two specification sections?

[wchargin]

@SamWilsn: Exactly; thank you. Switched to ‖, and also changed structHash to hashStruct(message) for consistency with other parts of the document.

[Pandapip1]

I would prefer if you also removed the first Specification section.

[SamWilsn]

I would prefer if you also removed the first Specification section.

Like the whole section, or just the header?

[Pandapip1]

Like the whole section, or just the header?

The whole first specification section, unless I misread it, is redundant.

[xinbenlv]

In the #5475 two other Editors voiced they prefer Option 2: starts a new EIP with new number but keep EIP-712's current specification.

Would it be ok to hold merging this PR until that consensus converges?

[wchargin]

No, sorry, I won't be starting a competing EIP with this fix because that is preposterous. This is obviously a simple copy-and-paste error. The text is inconsistent with itself, with all implementations, and with the clear intent of the EIP. The consequences of starting a new "EIP-713" with this fix and trying to get everyone to "migrate" to that are so much worse than the alternatives that it makes no sense to seriously discuss them. The only advantage to that plan is that it follows the process for process's sake, sticks to consistency for consistency's sake, without ever taking a step back to consider the purposes of that process and the impact of the various options. I won't be party to that.

@xinbenlv: Thanks for clarifying that you're not an official editor. If the EIP-712 editors want to merge this patch, great. If the editors would prefer to abandon it and leave the EIP inconsistent, well, it'll be a bit confusing, but no one will die. I don't have the energy or inclination to debate this endlessly, but I'm happy to help shepherd this through if there's anything needed from me, and happy to close it if that's the will of the council. Cheers.

[Pandapip1]

It's still not clear what the consensus among editors is. CC @gcolvin @axic @lightclient

[SamWilsn]

It's still not clear what the consensus among editors is.

Consensus was reached and documented in this thread.

We aren't waiting on editors to approve, but rather one of @Recmo, @LogvinovLeon, or @dekz.

[fulldecent]

This is a very important EIP. I would like to see an erratta section at least recognizing this change, if not explaining it.

[github-actions[bot]]

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[minebuu]

I still don't know why these critical errors haven't been fixed. EIP712 is a very important document and can resolve ambiguity with many examples already implemented, but it is very confusing for newbie.

I hope to see that change soon.

[Pandapip1]

We aren't waiting on editors to approve, but rather one of @recmo, @LogvinovLeon, or @dekz.

@recmo, @LogvinovLeon, or @dekz, please approve this PR.

[github-actions[bot]]

There has been no activity on this pull request for 2 weeks. It will be closed after 3 months of inactivity. If you would like to move this PR forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[Pandapip1]

CC @recmo, @LogvinovLeon, @dekz