signTypedData for arrays

[cag]

This builds on @bitpshr's previous work on signTypedData, adding support for the following:

Arrays of atomic types have been encoded as a hash of its contents, where its contents are word-aligned according to the type.

Arrays of dynamic types have been encoded as a hash of each element's hash.

Arrays of structs retain their encoding as a hash of its contents' concatenated hashStructs.

Arrays of arrays are encoded recursively, as a hash of the contained arrays' hash encodings concatenated.

[cag]

I've decided to turn missing values into an error. I'm not sure if it is proper to do so, or if values may be optional and skipped (how the implementation was before). If a decision is made on this, maybe a test case should be added to reflect that decision?

[danfinlay]

This is probably worth asking on the EIP's discussion, to gauge for strong opinions.

[cag]

Okay I've done so: https://ethereum-magicians.org/t/eip-712-eth-signtypeddata-as-a-standard-for-machine-verifiable-and-human-readable-typed-data-signing/397/19?u=cag

[cag]

Type dependencies were not correctly being discovered for types that have arrays of structs. Previously, the test suite passed only because the Mail struct has both Person and Person[] fields. If it only had Person[] fields, its encodeType would return Mail(Person[] to,string content), missing the Person dependency.

That issue is also addressed in e0d30b1

[danfinlay]

This is looking good, and so I'm really just trying to find the basic diligence we should exercise to merge. Another thing that came to mind is that this adds specificity to an existing EIP, and so it seems to merit submitting either a new EIP that depends on EIP-712 (with the official discussion directed at your magicians link), or an addition to EIP-712.

[cag]

Thanks for your support! I gave this some thought and added some of my thoughts to the thread. Hopefully I will find the time to write a patch to the EIP or an EIP extension, but this can sit here as long as it needs to.

[danfinlay]

What if we just merge this as signTypedData_v4? That way we don't have to wrestle with whether this is an appropriate breaking change, and we avoid breaking changes entirely.

[cag]

Hey Dan, I've been pretty busy with other stuff, so I haven't had time to properly make a PR for EIP-712 yet. I think using signTypedData_v4 for now might be reasonable, though I may finally have some bandwidth to author a change to EIP-712 specifying this behavior. I think this would be neat to have in time for the MetaCartel meet too...

[danfinlay]

The thing is, changes to EIP-712 are not really binding nor are they easy to coordinate with wallet devs. the _v[x] suffix is made exactly so that we can neatly define variants. I think it's probably the right path here, because if you submit a change to EIP-712, there's still no guarantee that other wallets implement/support that specific change, so it's best to specify exactly the namespace that this definition will fall under.

[cag]

Alright, I've decided to place the new implementation behind a useV4 flag parameter internally, and have implemented signTypedData_v4 and recoverTypedSignature_v4.

I've decided to have the internal TypedDataUtils default to the v4 implementation. I figured this was okay since this isn't a documented item, and things are mostly backwards compatible anyway. If you think useV4 should default to false instead on the internal API, let me know, and I'll make that change.

[cag]

Wait, I did remember sort of remembering this, but doesn't the current EIP-712 draft actually cover the cases that are mentioned wrt arrays and recursive stuff? I think this implementation actually diverges from the current EIP 712.

Anyway, for that reason, I'd appreciate it if you could pause on this PR while I try to sort this out.

For reference:

The array values are encoded as the keccak256 hash of the concatenated encodeData of their contents (i.e. the encoding of SomeType[5] is identical to that of a struct containing five members of type SomeType).

The struct values are encoded recursively as hashStruct(value). This is undefined for cyclical data.

---

EDIT: Nevermind. I was concerned about nested structs not having their typeHash be a part of the hash payload somewhere, but I was wrong about diverging from the current EIP:

• hashStruct(s : 𝕊) = keccak256(typeHash ‖ encodeData(s)) where typeHash = keccak256(encodeType(typeOf(s)))

...

The struct values are encoded recursively as hashStruct(value). This is undefined for cyclical data.

I guess maybe the standard could use some clarification on some cases. I'll open a PR there still.

[cag]

I've added this into my EIP edit, which is not accounted for in this PR yet (this PR will error on those inputs instead, but recursive structures are part of the EIP spec):

The struct values, when present, are encoded recursively as hashStruct(value). In order to support recursive data structures, struct values may be absent. Absent struct values are encoded as bytes32(0). Encoding is undefined for cyclical data.

[danfinlay]

I'll pause, but on the upside, diverging is a great reason to have your own namespace :)

Also a great reason to draft a fresh EIP for that namespace... ;)

[cag]

Haha, that might be true, but AFAIK I haven't broken compatibility with existing specified behavior, I've just specified more specific behavior. Also, in the process, I've come across this case I had not considered previously:

In addition to the changes quoted above, I've made all specified struct members mandatory except for structs. The reason why structs are not mandatory is because I've realized that the standard intended for recursive types to be possible. As an example, consider:

struct Node {
    bytes data;
    Node next;
}

This was a valid type in the standard, but it wasn't clear how to encode instances of this type. With mandatory struct members, this type would not be possible to express. I've taken the liberty to state that given the example, the next field may either be encoded recursively as its structHash, or as bytes32(0) when expressing its absence from an instance.

[danfinlay]

Oh that's great, because recursive types are a requirement for one of my favorite things that has yet to be built.

Could you include a test demonstrating recursive use of this API? After that, I think this looks good to merge if you agree.

[cag]

I've changed the implementation and included a tree as a test vector. I think this is fine to merge if you're ready.

[danfinlay]

Published to npm as eth-sig-util@2.3.0!

[danfinlay]

Hey, do you know if your contribution here fixed #56?

[cag]

Off the cuff, I'd guess that this PR doesn't affect that issue, as the message in that format should be hashed the same way in both v3 and v4. It could be an implementation detail though...

[k26dr]

Is there an example somewhere that shows how to verify structs with arrays in a smart contract? The example provided in the blog post only covers structs with non-array data types.

https://weijiekoh.github.io/eip712-signing-demo/index.html

[cag]

Unfortunately, as far as I can tell, there isn't an example anywhere of doing this in Solidity. Actually, the signing demo page seems to be broken for me, as I don't even see the source that you're supposed to copy into Remix there...