Update 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:

• EIPs/EIPS/eip-712.md

  Line 82 in 6fe3596

  * `encode(message : 𝔹⁸ⁿ) = "\x19Ethereum Signed Message:\n" ‖ len(message) ‖ message` where `len(message)` is the _non-zero-padded_ ascii-decimal encoding of the number of bytes in `message`.

• EIPs/EIPS/eip-712.md

  Line 87 in 6fe3596

  The encoding is compliant with [EIP-191][EIP-191]. The 'version byte' is fixed to `0x01`, the 'version specific data' is the 32-byte domain separator `domainSeparator` and the 'data to sign' is the 32-byte `hashStruct(message)`.

  
  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?

[SamWilsn]

This is pretty clearly a copy-paste error from eth_sign's API documentation. No one has implemented eth_signTypedData as an alias for eth_sign.

[SamWilsn]

The rest of the document uses ‖ for concatenation:

Suggested change

	The sign method calculates an Ethereum specific signature with: `sign(keccak256("\x19\x01" + domainSeparator + structHash))`, as defined in the "Specification" section above.
	The sign method calculates an Ethereum specific signature with: `sign(keccak256("\x19\x01" ‖ domainSeparator ‖ structHash))`, as defined above.

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

[Pandapip1]

@SamWilsn @lightclient @axic @gcolvin are we okay with manually merging this?

[github-actions]

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]

Dismiss stale bot.

[github-actions]

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]

Dismissing stale bot.

[github-actions]

The commit 75be219 (as a parent of 050d24f) contains errors.
Please inspect the Run Summary for details.

[Pandapip1]

@eth-bot rerun