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 clearingTimes #72

Merged
merged 5 commits into from Feb 7, 2019

Conversation

Projects
None yet
2 participants
@dteiml
Copy link
Contributor

dteiml commented Jan 4, 2019

No description provided.

@dteiml

This comment has been minimized.

Copy link
Contributor Author

dteiml commented Jan 4, 2019

Solves #63 . Solves everything there except documentation because Overleaf History is empty (I sent email to support). Tests haven't been changed.

@dteiml dteiml changed the base branch from develop to release/2.0 Jan 7, 2019

@dteiml dteiml requested a review from anxolin Jan 7, 2019

@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 7, 2019

@anxolin anxolin requested review from dasanra and josojo Jan 7, 2019

@anxolin
Copy link
Contributor

anxolin left a comment

I added a few comments.

Also, since we added a new variable, I think we need to add new assertions for the tests, and maybe new tests.

Show resolved Hide resolved contracts/DutchExchange.sol Outdated
Show resolved Hide resolved contracts/DutchExchange.sol Outdated
Show resolved Hide resolved contracts/DutchExchange.sol Outdated
Show resolved Hide resolved contracts/DutchExchange.sol Outdated
@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 11, 2019

@dteiml , I cherry picked from another branch the commit that re-enables the tests also for branches.

I think is better to see before merging if the tests passes.

@anxolin anxolin force-pushed the feature/addClearingTimes branch 2 times, most recently from 4a08b78 to d77ed1b Jan 11, 2019

@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Jan 11, 2019

I just added some commits to fix some issues with the tests.

Also I rebased the release/2.0 to get the latest merged changes and make it easier to merge later...

@anxolin anxolin force-pushed the feature/addClearingTimes branch from 8b18505 to d8f7150 Jan 14, 2019

@anxolin
Copy link
Contributor

anxolin left a comment

I don't find assertions for the post buy order.

We need to ensure that the right date is set for normal clearings and for close theoreticals.

Show resolved Hide resolved contracts/DutchExchange.sol Outdated
@anxolin

This comment has been minimized.

Copy link
Contributor

anxolin commented Feb 4, 2019

@dteiml can you rebase your branch with release/2.0 or merge release/2.0 into your branch, so we can review if we should merge this branch?

Thanks!!

@anxolin

anxolin approved these changes Feb 7, 2019

@anxolin anxolin merged commit 1d33ac4 into release/2.0 Feb 7, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details

@anxolin anxolin deleted the feature/addClearingTimes branch Feb 7, 2019

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