Skip to content
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

MarketCollateralPool Test #166

Merged
merged 11 commits into from Dec 12, 2018
Merged

Conversation

perfectmak
Copy link
Contributor

@perfectmak perfectmak commented Dec 7, 2018

Description

Tests for Market Collateral Pool minting, redeeming and settle and close.

Checklist
  • Linter status: 100% pass
  • Changes don't break existing behavior
  • Tests coverage hasn't decreased
Refers/Fixes

Fixes: #159
Fixes: #160

@perfectmak perfectmak force-pushed the feature/test-MarketCollateralPool branch 2 times, most recently from 8de0f8f to c0e9313 Compare December 7, 2018 19:51
@rojosnow
Copy link
Contributor

rojosnow commented Dec 7, 2018

@perfectmak Is this WIP PR tied to a specific issue? If so, go ahead and connect it to the PR. If not, set the Estimate and the current Milestone. Thanks!

@rojosnow
Copy link
Contributor

rojosnow commented Dec 7, 2018

I found the issue and connected it!

@0xean
Copy link
Contributor

0xean commented Dec 8, 2018

@perfectmak - rebase when you have a chance, everything is deploying properly and tests can now be ran.

The first basic minting test works, so hopefully, now your able to move forward without dealing with the complexities before!

@perfectmak perfectmak force-pushed the feature/test-MarketCollateralPool branch from c0e9313 to cae6d80 Compare December 10, 2018 16:55
@coveralls
Copy link

coveralls commented Dec 10, 2018

Pull Request Test Coverage Report for Build 652

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+6.5%) to 84.665%

Totals Coverage Status
Change from base Build 631: 6.5%
Covered Lines: 305
Relevant Lines: 343

💛 - Coveralls

@0xean
Copy link
Contributor

0xean commented Dec 11, 2018

@perfectmak - reviewing and changing removing WIP from title

@0xean 0xean changed the title WIP: MarketCollateralPool Test MarketCollateralPool Test Dec 11, 2018
Copy link
Contributor

@0xean 0xean left a comment

Choose a reason for hiding this comment

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

@perfectmak this looks very good!

Nice work with how you are forcing the contracts to settlement, I think this is much cleaner than our old approach.

I think we are still missing a few tests. Let me confirm and add another comments if so.

// 3. force contract to settlement
const settlementPrice = await settleContract();

// 4. redeem all shorts on settlement
Copy link
Contributor

Choose a reason for hiding this comment

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

@perfectmak should we do the same thing here for the long tokens to ensure it works for both sides (long, short) sides of the market.

@@ -21,95 +21,237 @@ contract('MarketCollateralPool', function(accounts) {
const entryOrderPrice = 33025;
Copy link
Contributor

Choose a reason for hiding this comment

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

some of these vars are no longer used, can we clean them up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, cleaning those up now.

@0xean
Copy link
Contributor

0xean commented Dec 11, 2018

Missing tests (I think)

  • mintPositionTokens fails if market contract is already settled
  • mintPositionTokens TokensMinted event is fired and correctly returns values
  • redeemPositionTokens TokensRedeemed event is fired and correctly returns values
  • settleAndClose burns the expected number of position tokens upon success
  • settleAndClose TokensRedeemed event is fired and correctly returns values

I also think we should consider looking at the totalSupply_ of the erc20 short and long tokens to make sure that is being accounted for correctly, but perhaps @eswarasai is tackling that in his tests of PositionToken.sol

@eswarasai
Copy link
Contributor

@pelsasser @perfectmak -- So the tests in regards with the PositionToken.sol are more of the duplicate to that of CollateralPool.test.js. It makes sense for me to have all the passing/failing tests in regards with the PositionToken to be under CollateralPool since it's the caller of the Long/Short Position Tokens. I'll try to tackle few of the above tests on this branch to breakdown the tasks. Thanks!

@CLAassistant
Copy link

CLAassistant commented Dec 12, 2018

CLA assistant check
All committers have signed the CLA.

@perfectmak
Copy link
Contributor Author

perfectmak commented Dec 12, 2018

@pelsasser I believe all the case should have been checked now. Kindly take a look.

@0xean 0xean merged commit 889ded5 into feature/tokenized Dec 12, 2018
@0xean 0xean deleted the feature/test-MarketCollateralPool branch December 12, 2018 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants