-
-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EIP-712: Array support #22
Conversation
@@ -318,17 +318,17 @@ test('signedTypeData', (t) => { | |||
const sig = sigUtil.signTypedData(privateKey, { data: typedData }) | |||
|
|||
t.equal(utils.encodeType('Mail', typedData.types), | |||
'Mail(Person from,Person to,string contents)Person(string name,address wallet)') | |||
'Mail(Person from,Person[] to,string contents)Person(string name,address wallet)') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend adding an independent test for this, to avoid changing the test values provided in the EIP, this way it is also ensured that the previous behaviour did not break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @rmeissner, thanks for the feedback. I submitted an upstream PR to update the EIP test values (see ethereum/EIPs#1242.) While we don't plan to land this PR until that upstream PR also lands, you're right that separate tests would be more robust. Will update.
@@ -16,7 +16,7 @@ const typedData = { | |||
], | |||
Mail: [ | |||
{ name: 'from', type: 'Person' }, | |||
{ name: 'to', type: 'Person' }, | |||
{ name: 'to', type: 'Person[]' }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when creating the typeHash a lookup is performed based on the type. In this case the type would be Person[]
, so it would not match Person
in the types dictionary.
In the test this works since from
references Person
, but would be interesting if it still works when we remove that reference.
t.equal(ethUtil.bufferToHex(utils.encodeData(typedData.primaryType, typedData.message, typedData.types)), | ||
'0xa0cedeb2dc280ba39b857546d74f5549c3a1d7bdc2dd96bf881f76108e23dac2fc71e5fa27ff56c350aa531bc129ebdf613b772b6604664f5d8dbe21b85eb0c8cd54f074a4af31b4411ff6a60c9719dbd559c221c8ac3492d9d872b041d703d1b5aadf3154a261abdd9086fc627b61efca26ae5702701d05cd2305f7c52a2fc8') | ||
'0xdd57d9596af52b430ced3d5b52d4e3d5dccfdf3e0572db1dcf526baad311fbd1fc71e5fa27ff56c350aa531bc129ebdf613b772b6604664f5d8dbe21b85eb0c872ea07cf404427eb52aed74d9998e238434f046d95c4ff7802d628628bf77a16b5aadf3154a261abdd9086fc627b61efca26ae5702701d05cd2305f7c52a2fc8') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When implementing this in python it seems like that these test values are wrong.
To verify this I created a smart contract on remix:
pragma solidity 0.4.24;
contract Test {
function personData()
public
pure
returns (bytes)
{
bytes32 personSchemaHash = keccak256("Person(string name,address wallet)");
return abi.encode(
personSchemaHash,
keccak256('Bob'),
address(0xbBbBBBBbbBBBbbbBbbBbbbbBBbBbbbbBbBbbBBbB)
);
}
function signData()
public
pure
returns (bytes)
{
bytes32 personSchemaHash = keccak256("Person(string name,address wallet)");
return abi.encode(
0xdd57d9596af52b430ced3d5b52d4e3d5dccfdf3e0572db1dcf526baad311fbd1,
keccak256(
abi.encode(
personSchemaHash,
keccak256('Cow'),
address(0xCD2a3d9F938E13CD947Ec05AbC7FE734Df8DD826)
)
),
keccak256(personData()),
keccak256("Hello, Bob!")
);
}
function signHash()
public
pure
returns (bytes32)
{
return keccak256(signData());
}
function sign()
public
pure
returns (bytes32)
{
return keccak256(abi.encodePacked(
byte(0x19),
byte(1),
0xf2cee375fa42b42143804025fc449deafd50cc031ca257e0b194a650a912090f,
signHash()
));
}
}
The issue seems to be in the code where the array data is concatenated (see comment above)
throw new Error('Arrays currently unimplemented in encodeData') | ||
encodedTypes.push('bytes32') | ||
const parsedType = field.type.slice(0, field.type.lastIndexOf('[')) | ||
value = ethUtil.sha3(value.map(item => this.encodeData(parsedType, item, types)).join('')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seems that join
converts this into a string and result in a wrong hash (see comment in tests)
To fix this use:
Buffer.concat(value.map(item => this.encodeData(parsedType, item, types)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see the comments posted by rmeissner
addressed before merging.
Closing this for now and will re-open when EIP-712 array support firms up. Per the proposal's author:
|
The original implementation of
signTypedData
omitted array support. This pull request remedies this by adding array support as per EIP-712 along with updated tests.