Update Order Hash Algo to be EIP712 Compliant #276
Conversation
| pure | ||
| returns (bytes32) | ||
| { | ||
| return EIP712Library.hashEIP712Message( |
There was a problem hiding this comment.
Note that the order hash now is doubly hashed with the EIP712 Domain
| pure | ||
| returns(bytes32) | ||
| { | ||
| // Hash the order parameters | ||
| return keccak256( | ||
| abi.encodePacked( | ||
| EIP712_ORDER_SCHEMA_HASH, // EIP 712 order schema hash |
There was a problem hiding this comment.
Note that the order hash now includes the schema hash
| /* ============ Constants ============ */ | ||
|
|
||
| // EIP191 header for EIP712 prefix | ||
| string constant internal EIP191_HEADER = "\x19\x01"; |
There was a problem hiding this comment.
I don't think these are being used?
There was a problem hiding this comment.
Technically, it is being represented in the assembly on line 88.
There was a problem hiding this comment.
Or leave it in a comment where it's being used
contracts/core/lib/EIP712Library.sol
Outdated
| * @param hashStruct The EIP712 hash struct. | ||
| * @return EIP712 hash applied to this EIP712 Domain. | ||
| */ | ||
| function hashEIP712Message(bytes32 hashStruct) |
There was a problem hiding this comment.
Put parameter on its own line
contracts/core/lib/EIP712Library.sol
Outdated
| { | ||
| bytes32 eip712DomainHash = EIP712_DOMAIN_HASH; | ||
|
|
||
| // Assembly for more efficient computing: |
|
|
||
| mstore(memPtr, 0x1901000000000000000000000000000000000000000000000000000000000000) // EIP191 header | ||
| mstore(add(memPtr, 2), eip712DomainHash) // EIP712 domain hash | ||
| mstore(add(memPtr, 34), hashStruct) // Hash of struct |
There was a problem hiding this comment.
Where does 34 come from? domain hash plus header?
There was a problem hiding this comment.
Its 2 (previous) + 32 bytes
contracts/core/lib/EIP712Library.sol
Outdated
| * | ||
| * @return bytes32 Hash of the EIP712 Set Protocol Domain | ||
| */ | ||
| function getEIP712DomainHash() internal view returns (bytes32) { |
There was a problem hiding this comment.
What do you mean by this?
There was a problem hiding this comment.
Signature does not match our other function signatures. Standardize
| // Hash for the EIP712 Order Schema | ||
| bytes32 constant public EIP712_ORDER_SCHEMA_HASH = keccak256( | ||
| abi.encodePacked( | ||
| "IssuanceOrder(", |
There was a problem hiding this comment.
What happened to adding the coreAddress?
There was a problem hiding this comment.
The Domain Separator schema fields are all optional. The address is a good way to delineate further between dapps, but isn't really required. It also adds unnecessary complexity atm.
| * | ||
| * @return bytes32 Hash of the Issuance Order Schema | ||
| */ | ||
| function getEIP712OrderSchemaHash() internal view returns (bytes32) { |
contracts/core/lib/EIP712Library.sol
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| pragma solidity 0.4.24; |
| }); | ||
|
|
||
| describe('#getEIP712DomainHash', async () => { | ||
| const expectedEIP712Hash: Bytes = '0xa8dcc602486c63f3c678c9b3c5d615c4d6ab4b7d51868af6881272b5d8bb31ff'; |
There was a problem hiding this comment.
nit: can you show how this was generated? Is it from a method in utils?
There was a problem hiding this comment.
It was done in a round-about way. Used the contracts to generate the hash, then updated utils (and its associated unit tests)
| it('should return the correct hash', async () => { | ||
| const expectedHash = '0x5686079a65f95107943e531f6f7f755044148600233246c75fdce6e59c85cae5'; | ||
| const returnedHash = await subject(); | ||
| expect(returnedHash).to.equal(expectedHash); |
There was a problem hiding this comment.
nit:
const hash = await subject()
const expectedHash = blah
assert hash = expectedHash
felix2feng
force-pushed
the
felix/signature-abstraction
branch
from
de8fc6d
to
ee329d1
Compare
Pull Request Test Coverage Report for Build 2694
💛 - Coveralls |
This is the first PR to start building abstracting out signatures.