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

Add coordinator wrapper #1792

Merged
merged 55 commits into from
May 9, 2019
Merged

Add coordinator wrapper #1792

merged 55 commits into from
May 9, 2019

Conversation

xianny
Copy link
Contributor

@xianny xianny commented Apr 25, 2019

Description

Adds a wrapper for handling Coordinator transactions

Testing instructions

cd packages/contract-wrappers && yarn build && yarn test

Unit tests cover fills, soft cancels, hard cancels, and handling server errors.

Types of changes

  • New feature (non-breaking change which adds functionality)

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.

@xianny xianny requested review from fabioberger and dekz April 26, 2019 03:55
@@ -39,6 +39,7 @@
},
"devDependencies": {
"@0x/contracts-test-utils": "^3.1.2",
"@0x/coordinator-server": "^0.0.6",
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this is bringing in some old dependencies of contract-wrappers, might result in some strange behaviour.

@xianny
Copy link
Contributor Author

xianny commented Apr 30, 2019

I'd still like to write more tests for error cases to check the format of the errors, but apart from that I've implemented all the changes brought up.

I changed everything to return Promise<txhash | CoordinatorError> at first, then changed it back to throw errors. But had to stringify CoordinatorError objects. Feeling ambivalent about this. If we return instead of throwing, it breaks the resolve/reject interface for Promises. Is there a way to throw an error with an object instead of just a string?

@xianny xianny removed the request for review from albrow May 9, 2019 02:03
Copy link
Contributor

@fabioberger fabioberger left a comment

Choose a reason for hiding this comment

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

LGTM once CI tests pass

@xianny
Copy link
Contributor Author

xianny commented May 9, 2019

static-tests-python is failing everywhere else too. I'm going to call it unrelated and merge this. 🎉

@xianny xianny merged commit 41cc523 into development May 9, 2019
@xianny xianny deleted the feature/coordinator-wrapper branch May 9, 2019 22:48
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.

4 participants