Skip to content

Conversation

@PaulRBerg
Copy link
Contributor

@PaulRBerg PaulRBerg commented Sep 29, 2018

This PR implements EIP-191 and EIP-712.

Additions

  1. Defined a list of content types as per the table below
  2. Written a specific function for each EIP
  3. Implemented the ecRecover method in the clef API

Content Types

This fully respects the EIP-191 specs.

Version byte Name Description
0x00 text/validator Data with intended validator
0x01 data/typed EIP-712 typed data
0x02 application/clique Clique headers
0x45 text/plain Plain, personal messages

Notes

  1. Everything was coded in clef, there will be no work done to retrofit the implementation on geth and eth_sign will be deprecated
  2. Reused the clique functions written by @holiman in the geth_relay branch.

TODO

  • Implement data/typed (EIP712)
  • Write tests for data/typed
  • Implement text/validator
  • Write tests for text/validator

@PaulRBerg
Copy link
Contributor Author

The last update also reformatted the code with go fmt.

@holiman
Copy link
Contributor

holiman commented Oct 11, 2018

Heh, we usually rebase instead of mega-merge :)

@PaulRBerg
Copy link
Contributor Author

Just rebased now :)

@holiman holiman requested review from holiman and removed request for acud, fjl, gbalint, holisticode, janos, karalabe, nolash, nonsense, zelig and zsfelfoldi October 12, 2018 14:57
@PaulRBerg PaulRBerg changed the title [WIP] Implemented EIP191 in clef [WIP] Implemented EIP191/712 in clef Oct 15, 2018
@holiman
Copy link
Contributor

holiman commented Feb 5, 2019

I've modified the rawData field now, and added some protections against panics due to type assertions that were not checked.

@holiman
Copy link
Contributor

holiman commented Feb 5, 2019

Travis finally succeeded. Will merge this later tonight, when I have some time to sit down some more with it and extensively test after the merge

@PaulRBerg
Copy link
Contributor Author

Hey @holiman, many thanks for rebasing to the new wallet API :)

But I'm wondering what use there is of ever having that map there...

I originally implemented it like that because I thought it helped with the pretty printing functionality.

Hooray, happy to see Travis passing! Just for the record, is there a known issue with AppVeyor? Afaik, all PRs fail there.

@holiman
Copy link
Contributor

holiman commented Feb 6, 2019

Just for the record, is there a known issue with AppVeyor? Afaik, all PRs fail there.

Yup, there is.

@DGideas
Copy link

DGideas commented Mar 19, 2025

From https://github.com/ethereum/go-ethereum/pull/17789/files#diff-8e03bbbabe1aac9858b32fe8596aef7c69cfbc6cdbd05a5905d05451b5e42785R385-R388

func (typedData *TypedData) EncodeData(primaryType string, data map[string]interface{}, depth int) (hexutil.Bytes, error) {
...
	// Verify extra data
	if len(typedData.Types[primaryType]) < len(data) {
		return nil, errors.New("there is extra data provided in the message")
	}
...

In the case where a message contains extra fields beyond what the type itself defines, I did some research on this:

  • EIP-712 does not clearly define the detailed rules for retrieving typedData.
  • The Solidity code provided in EIP-712 does not include such a check.
  • The JavaScript example code from the EIP-712 attachments ignores extra fields in the message and still passes.
  • In go-ethereum, the test cases under signer/core/testdata/expfail_* do not contain any failed cases related to extra fields in the message object.

So, I am curious whether this check follows the EIP-712 documentation or if go-ethereum imposes stricter validation than EIP-712 itself. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants