Generalized Perp Vaults #47
Conversation
contracts/core/OpynPerpVault.sol
Outdated
| // keep track of underlying balance after | ||
| uint256 totalUnderlyingAfterDeposit = totalUnderlyingAsset(); | ||
| require(totalUnderlyingAfterDeposit < cap, 'O7'); | ||
| require(totalUnderlyingAfterDeposit.sub(totalUnderlyingBeforeDeposit) == amount); |
There was a problem hiding this comment.
throw an error like in line 192 if it fails!
Also refer to the error codes in line 18 and add the error to it, make sure error codes are still valid, if not remove them.
contracts/core/OpynPerpVault.sol
Outdated
|
|
||
| // keep track of underlying balance after | ||
| uint256 totalUnderlyingAfterWithdrawal = totalUnderlyingAsset(); | ||
| require(totalUnderlyingBeforeWithdrawal.sub(totalUnderlyingAfterWithdrawal) == underlyingShareOfRecipient); |
There was a problem hiding this comment.
add an error message :)
contracts/core/OpynPerpVault.sol
Outdated
| // send underlying to user | ||
| underlyingToken.safeTransfer(msg.sender, underlyingOwedToUser); | ||
| // withdraw underlying from vault | ||
| uint256 underlyingShareOfRecipient = _getWithdrawAmountByShares(_share); |
There was a problem hiding this comment.
This is the amount of underlying to transfer right? calling it share is a bit confusing, can you rename it to maybe underlyingToTransfer / something else?
There was a problem hiding this comment.
done. changed names to underlyingToRecipientBeforeFees and underlyingToRecipientAfterFees.
| vaultTotal, 'internal balance is incorrect' | ||
| ); | ||
| // check the underlying token balances | ||
| expect(vaultTotal).to.be.equal(UnderlyingBeforeDeposit.add(p1DepositAmount), 'internal underlying balance is incorrect'); |
There was a problem hiding this comment.
can you also check that vaultTotal == underlying.balanceOf(vault.address) ?
There was a problem hiding this comment.
this is true for p1 and p2 (and generally for any depositor before rollover) but not for p3. Added a manual check section that checks for this for p1 and p2 and does the necessary sanity check for p3 (reserve*(p1+p2)+p3 == underlying.balanceOf(vault.address) ) .
| sdcrvRenWsbtcPrice.toNumber() | ||
| ); | ||
| expect((await vault.balanceOf(depositor2.address)), 'incorrect amount of shares minted').to.be.equal(totalSharesMinted); | ||
| expect(totalSharesExisting, 'incorrect amount of shares existing').to.be.equal(sharesBefore.add(totalSharesMinted)); |
There was a problem hiding this comment.
can you also check that vaultTotal == underlying.balanceOf(vault.address) ?
| ).to.be.true; | ||
|
|
||
| expect((await action1.lockedAsset()).eq(underlyingExpectedBeforeMintInActionLocked),'collateral should not be locked').to.be.true; | ||
| expect((await wbtc.balanceOf(action1.address)).eq(underlyingExpectedBeforeMintInActionUnlocked),'collateral should all be unlocked').to.be.true; |
There was a problem hiding this comment.
can you also check that the action.currentVault is underlyingExpectedBeforeMintInActionUnlocked?
There was a problem hiding this comment.
done (I guess you meant currentValue())
|
|
||
| expect(sharesBefore, 'incorrect amount of shares withdrawn').to.be.equal(sharesAfter.add(sharesToWithdraw)) | ||
| // check shares | ||
| expect(sharesAfter, 'incorrect amount of shares withdrawn').to.be.equal(sharesExpectedAfter); | ||
|
|
There was a problem hiding this comment.
check that vault.balanceOf(depositor1) is 0 (since they withdrew all so they should have no shares)
There was a problem hiding this comment.
done. For all 3 users (since they all deposit all their wallet amount to the strategy and then withdraw it all) added a check user shares section which verifies that they are indeed 0 after withdrawal.
| vaultSdECRVBalanceBefore.sub(sdcrvRenWsbtcWithdrawn).sub(1) as any, | ||
| vaultSdECRVBalanceBefore.sub(sdcrvRenWsbtcWithdrawn).add(1) as any, | ||
| ); | ||
| expect(underlyingAfterWithdrawal, 'incorrect underlying remained in the vault').to.be.eq(underlyingExpectedAfterWithdrawal); |
There was a problem hiding this comment.
check that amountWithdrawn = p1DepositorAmount + p1's share of premium.
There was a problem hiding this comment.
also check that p1 ended up in a profitable position despite fees. (basically if mint and sell didn't happen, this test should fail and p1 should not have made a profit)
There was a problem hiding this comment.
done. added a manual checks on profitability section on all 3 users with the above 2 tests. p1 and p2 are the same and follow the above comment's logic, p3 just loses the withdrawal fee since entrance was after rollover & mint so has no profit.
* loss changes completed - 2 testings left to verify * performance fee added * Profit still in progress * updates * bignumber hack * KG commit through Demola's Branch - fixed pricer, completed all calculations for profit scenario * KG new commit through Demola's Branch - fixed some tests to work for loss too, fixed type of numbers for certain calculations, first cleanup for profit scenario Co-authored-by: Konstantinos Gkolias <[email protected]>
contracts/core/OpynPerpVault.sol
Outdated
|
|
||
| emit Withdraw(msg.sender, underlyingOwedToUser, _share); | ||
| } | ||
| console.log("Fee Recipient: %s",feeWithdrawalRecipient); |
There was a problem hiding this comment.
remove console.logs :)
…eaning - Ready for review
| console.log("premium:",premium.toString()); | ||
| console.log("underlyingBeforeRollOverTotal:",underlyingBeforeRollOverTotal.toString()); | ||
| console.log("underlyingExpectedAfterMintTotal:",underlyingExpectedAfterMintTotal.toString()); | ||
| console.log("underlyingExpectedAfterMintInVault:",underlyingExpectedAfterMintInVault.toString()); | ||
| console.log("underlyingExpectedBeforeMintInActionLocked:",underlyingExpectedBeforeMintInActionLocked); |
There was a problem hiding this comment.
could we remove all the console logs and convert them to tests? :D
|
Aparna this version of the code exists on branch generalized_cc_vault without any console logs anywhere. I will keep kg_testing_2 as is for further testing in future if needed. |
No description provided.