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

Add ZeroExV2 Exchange Wrapper #439

Merged
merged 7 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@BrendanChou
Contributor

BrendanChou commented Sep 12, 2018

fixes #296

@BrendanChou BrendanChou requested a review from AntonioJuliano Sep 12, 2018

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 12, 2018

Pull Request Test Coverage Report for Build 7415

  • 43 of 43 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7351: 0.0%
Covered Lines: 1322
Relevant Lines: 1322

💛 - Coveralls

coveralls commented Sep 12, 2018

Pull Request Test Coverage Report for Build 7415

  • 43 of 43 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 7351: 0.0%
Covered Lines: 1322
Relevant Lines: 1322

💛 - Coveralls
@AntonioJuliano

Looks excellent, have a few comments

Show outdated Hide outdated contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol Outdated
Show outdated Hide outdated contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol Outdated
Show resolved Hide resolved contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol
Show resolved Hide resolved contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol
uint256 requestedFillAmount,
bytes orderData
)
external

This comment has been minimized.

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

I'm thinking about if this needs to be nonReentrant I don't think it does, but I'd like to be more convinced. I also see the V1 exchange wrapper is also not nonReentrant

@AntonioJuliano

AntonioJuliano Sep 13, 2018

Collaborator

I'm thinking about if this needs to be nonReentrant I don't think it does, but I'd like to be more convinced. I also see the V1 exchange wrapper is also not nonReentrant

Show outdated Hide outdated contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol Outdated
Show outdated Hide outdated contracts/margin/external/exchangewrappers/ZeroExV2ExchangeWrapper.sol Outdated
Show outdated Hide outdated migrations/2_deploy.js Outdated
Show outdated Hide outdated test/helpers/BytesHelper.js Outdated
Show outdated Hide outdated test/helpers/ZeroExV2Helper.js Outdated

BrendanChou added some commits Sep 13, 2018

@BrendanChou BrendanChou merged commit 419463d into master Sep 14, 2018

7 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: checkout_and_install Your tests passed on CircleCI!
Details
ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prod_build Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@BrendanChou BrendanChou deleted the bc_zevtwo branch Sep 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment