Add EIP712 support for bytes and uint formatted as string #427
Conversation
moisses89
requested review from
fmrsabino,
Uxio0 and
hectorgomezv
and removed request for
a team
Pull Request Test Coverage Report for Build 3882828086
💛 - Coveralls |
moisses89
changed the title
| {"name": "maxByte", "type": "bytes32"}, | ||
| ], | ||
| } | ||
| # test string uint (chainId) |
There was a problem hiding this comment.
nitpick: this comment does not apply to this test right?
There was a problem hiding this comment.
Yes, copy pasted from above, done fa4a21f
There was a problem hiding this comment.
@moisses89 I think these things should be addressed in a new PR
| @@ -57,6 +57,14 @@ def _encode_field(name, typ, value): | |||
| if value is None: | |||
| raise Exception(f"Missing value for field {name} of type {type}") | |||
|
|
|||
| # Accept string bytes | |||
| if "bytes" in typ and isinstance(value, str): | |||
| value = bytes.fromhex(value.replace("0x", "")) | |||
| if "bytes" in typ and isinstance(value, str): | ||
| value = bytes.fromhex(value.replace("0x", "")) | ||
|
|
||
| # Accept string uint |
There was a problem hiding this comment.
I think we should support any kind of integer, not only uint. This is a very misleading behaviour:
if "int" in typ
| @@ -4,6 +4,43 @@ | |||
|
|
|||
|
|
|||
| class TestEIP712(TestCase): | |||
| address = "0x8e12f01dae5fe7f1122dc42f2cb084f2f9e8aa03" | |||
There was a problem hiding this comment.
We should use checksummed addresses unless we are testing not checksummed addresses:
0x8e12f01DAE5FE7f1122Dc42f2cB084F2f9E8aA03
|
|
||
| self.assertEqual( | ||
| eip712_encode_hash(payload).hex(), | ||
| "2950cf06416c6c20059f24a965e3baf51a24f4ef49a1e7b1a47ee13ee08cde1f", |
There was a problem hiding this comment.
Why do you use hex() here? I think we should be consistent, either using hex() everywhere (it looks better and more readable IMHO) or using binary representation
There was a problem hiding this comment.
@moisses89 I think these things should be addressed in a new PR
Your suggestions sounds good for me, then I add the following PR #437
Regarding if use hex() or binary representation for test, I'd prefer hex() because as you mention is more readable.
Closes #424
Description
eip712_encode_hashis usingeth-abithat doesn't support the typebytesanduintformatted as string.Type
bytesandbytesNoneth-abionly accept python bytes, this was fixed including a conversion for bytes formatted on string to python bytes.Type
uintonly accept Numbers, this was fixed converting from string to python Integer.Additional changes