Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

Add transactionHashUtils to order-utils package #1576

Merged
merged 9 commits into from
Feb 5, 2019

Conversation

abandeali1
Copy link
Member

@abandeali1 abandeali1 commented Feb 2, 2019

Description

  • Adds transactionHashUtils to the order-utils package. The other way of hashing a tx is in the contract-wrappers package, but you really shouldn't require an exchange instance to simply hash a transaction.
  • Allows passing a domain into createTypedData to override the default.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@coveralls
Copy link

coveralls commented Feb 2, 2019

Coverage Status

Coverage increased (+0.06%) to 85.306% when pulling af4ed0f on feat/transaction-hash into 69c7c03 on development.

@abandeali1 abandeali1 changed the title Add transactionHashUtils to order-utils package [WIP] Add transactionHashUtils to order-utils package Feb 2, 2019
@abandeali1 abandeali1 force-pushed the feat/transaction-hash branch 3 times, most recently from 3f8141b to 9a77c15 Compare February 4, 2019 01:20
@abandeali1 abandeali1 changed the title [WIP] Add transactionHashUtils to order-utils package Add transactionHashUtils to order-utils package Feb 4, 2019
@abandeali1 abandeali1 requested a review from hysz February 4, 2019 06:25
@@ -1,6 +1,7 @@
{
"id": "/zeroExTransactionSchema",
"properties": {
"verifyingContractAddress": { "$ref": "/addressSchema" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required? If so, you should add it to the required array below.

} catch (error) {
if (_.includes(error.message, INVALID_TAKER_FORMAT)) {
const errMsg =
'Order taker must be of type string. If you want anyone to be able to fill an order - pass ZeroEx.NULL_ADDRESS';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ZeroEx.NULL_ADDRESS still a thing? Someone just using @0x/order-utils won't have that.

const typedData = eip712Utils.createZeroExTransactionTypedData(transaction);
const transactionHashBuff = signTypedDataUtils.generateTypedDataHash(typedData);
return transactionHashBuff;
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was any of this logic copy-pasted from the contract-wrappers transaction utilities? If so, can we merge them and make sure it's all DRY?

@abandeali1 abandeali1 force-pushed the feat/transaction-hash branch from d51531b to af4ed0f Compare February 5, 2019 17:26
@abandeali1 abandeali1 merged commit b5eb47f into development Feb 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants