Test/ Add buyCover revert unit tests #449
Conversation
roxdanila
force-pushed
the
test/cover-buycover-revert-unit-tests
branch
from
64176d4 to
0136d55
Compare
test/unit/Cover/buyCover.js
Outdated
| const period = 3600 * 24 * 364; // 30 days | ||
|
|
||
| const amount = parseEther('1000'); | ||
| const targetPriceRatio = '260'; |
There was a problem hiding this comment.
just checked and this pool fixture and the createStakingPool call ar all the same for all the tests currently.
It makes sense to have a beforeEach section which is just creating this default pool to avoid this code duplication.
We will add tests for buying across multiple pools. in those tests we can add the extra pool there but we always need at least this 1 pool.
initializing all these values in a fixture to be used in the rest of the test code would strip this section a code from each test.
There was a problem hiding this comment.
So let's do a beforeEach with all this code including the createStakingPool
create a fixture for the values for the pool so that values are not initialized in every test
There was a problem hiding this comment.
Thanks for the suggestion! I refactored the code, now the tests code is cleaner
| @@ -1,8 +1,7 @@ | |||
| const { expect } = require('chai'); | |||
| const { ethers } = require('hardhat'); | |||
There was a problem hiding this comment.
i'm not seeing a test for
Should revert if product is not initialized
should revert with Cover: Product not initialized
I see you mentioned you can't make it fail, see below 👇
There was a problem hiding this comment.
smart-contracts/test/unit/Cover/setup.js
Line 188 in 568e2f6
To make it revert with that message add a product like in the setup, which will have productId = 1
and set initialPriceRatio: 0
There was a problem hiding this comment.
Thanks for the help!
I tried doing that but it reverts with Cover: initialPriceRatio < GLOBAL_MIN_PRICE_RATIO which is set as a constant GLOBAL_MIN_PRICE_RATIO = 100.
Unless I'm missing something, it looks like products could not be in an uninitialized state, so in that case, Is that validation needed? I think @roxdanila mentioned something about new functionality to deprecate products in which case once introduced it would make sense to validate that the product was not deprecated
There was a problem hiding this comment.
Ah i see you're right. That validation was added very recently and that require is not useful anymore.
Thanks for clarifying.
Yeah we need that require removed it can't actually happen, and add a deprecation check instead
don't solve for this test in this PR
|
|
||
| const difference = daiBalanceBefore.sub(daiBalanceAfter); | ||
| bnEqual(difference, expectedPremium); | ||
| expect(difference).to.be.equal(expectedPremium); |
There was a problem hiding this comment.
thanks for fixing these
test/unit/Cover/helpers.js
Outdated
| assert.equal(storedCoverData.productId, productId); | ||
| assert.equal(storedCoverData.coverAsset, coverAsset); | ||
| bnEqual(storedCoverData.amountPaidOut, amountPaidOut); | ||
| expect(storedCoverData.amountPaidOut.toString()).to.be.equal(amountPaidOut); |
There was a problem hiding this comment.
you shouldn't need that toString() here, i expect
correct me if i'm wrong
There was a problem hiding this comment.
I was throwing an error, I updated the code so toString() is not longer needed
test/unit/Cover/helpers.js
Outdated
| assert.equal(segment.period, period); | ||
| assert.equal(segment.amount.toString(), amount.toString()); | ||
| bnEqual(segment.priceRatio, targetPriceRatio); | ||
| expect(segment.priceRatio.toString()).to.be.equal(targetPriceRatio); |
roxdanila
force-pushed
the
test/cover-buycover-revert-unit-tests
branch
from
0136d55 to
01caff4
Compare
Yes, the rest of the tests will be added in another PR |
Context
Partial implementation of #268
Changes proposed in this pull request
Adds new revert unit tests for
buyCover. Implemented some of the listed test cases, especially the reverts one.I couldn't find a way to test
Cover: Product not initialized, it seems that when a product is listed it also initializes the parameters so I couldn't reproduce a state of listed but withinitialPriceRatio = 0as the code is checking.Also, I removed the usage of
bnEqualin thebuyCovertests as it triggers a warningWarning: Use Hardhat Chai Matchers instead of bnEqual: https://hardhat.org/hardhat-chai-matchers/docs/overview#big-numbersTest plan
New tests were added in
test/unit/Cover/buyCover.jsChecklist
Review
When reviewing a PR, please indicate intention in comments using the following emojis: