Feature/mint #2
Feature/mint #2
Conversation
v0.0.208
There was a problem hiding this comment.
Testing is a bit too light for logic this intricate. Also you should definitely have unit tests and integration tests split out. Where the former is testing purely the function effects and the latter is testing deeper integration with functioning StakingRewards.
test/KSXVault.t.sol
Outdated
| // Asserts correct deposit at 1000 shares ratio | ||
| // Converts asset values to shares and deposits assets into the vault | ||
| function test_vault_deposit() public { | ||
| uint256 amount = 1 ether; | ||
| vm.startPrank(alice); | ||
| depositToken.approve(address(ksxVault), amount); | ||
| ksxVault.deposit(1 ether, alice); | ||
| assertEq(ksxVault.balanceOf(alice), amount * (10 ** ksxVault.offset())); | ||
| assertEq(stakingRewards.stakedBalanceOf(address(ksxVault)), amount); | ||
| vm.stopPrank(); | ||
| } |
There was a problem hiding this comment.
This test checks that Alice is minted the offset amount specified in the contract, but it does not verify that 1 Kwenta = 1000 KSX. Like if you set offset to 0 this test would still pass.
You should have a separate test that shows 1 KWENTA deposited results in 1000 KWENTA hardcoded. (original business case)
This should also be fuzzed. For your fuzz test you can multiply the initial KWENTA amount by 1000. You're trying to prove that for all amounts of Kwenta the initial ratio is 1:1000.
There was a problem hiding this comment.
Secondarily, you should have tests that test consecutive actions. For example, you should be testing
- Deposit 1 KWENTA receive x amount
- Deposit 2 KWENTA, now what amount do you receive? Test your hypothesis
and
- Deposit 1 KWENTA as Alice receive x amount
- Allow some time to pass and staking rewards to accrue in the contract*
- Deposit 1 KWENTA as Bob and confirm the expected amount
* more of an integration test but you need to have this
Your goal with test coverage is to test as much branching logic you can think of. This SupplySchedule.t is the gold standard for testing. Note how it even tests contract state at random points in the time in the future (future weeks expected behavior). Take some time to review this and look how many test cases were covered.
There was a problem hiding this comment.
Added new extensive tests for deposit process.
test/KSXVault.t.sol
Outdated
|
|
||
| // Withdraws a specified amount of assets from the vault by burning the | ||
| // equivalent shares | ||
| function test_withdraw() public { |
There was a problem hiding this comment.
Add a test that tests initial conversion ratio withdraw. ie. where Alice tries to withdraw 1 KWENTA and has to deposit 1000 KSX
There was a problem hiding this comment.
Secondarily you need more negative state tests. For example
- trying to withdraw with no shares
- trying to withdraw with more shares than you have
and do the same for deposits
A lot of this is already tested on ERC4626 but you modified the offset so you want to confirm that everything still works as intended. Also some modifications to stake/unstake
There was a problem hiding this comment.
This test file for the MultipleMerkleDistributor is another good study to understand what kind of negative tests I'm talking about. Don't leave stones unturned
There was a problem hiding this comment.
Added new extensive tests for redeeming process.
Implement Minting and Conversion Ratio to the KSX Vault
Description
See spec for Minting and Conversion Ratio :
https://github.com/Kwenta/kwenta-state-log/blob/master/kips/kip-123.md
Add tests to ensure the first $KSX is minted by depositing $KWENTA at an initial fixed ratio of 1,000 $KSX per $KWENTA.
Add KSXVaultBootsrap for tests