update yearn vault to v2 by chudnov · Pull Request #47 · ribbon-finance/ribbon-v2

chudnov · 2021-07-19T15:49:56Z

No description provided.

kenchangh · 2021-07-20T17:23:16Z

+        uint256 expiry;
+
+        // uninitialized state
+        if (closeParams.currentOption <= address(1)) {


I changed this recently, needs to be if (closeParams.currentOption == address(0)) {

kenchangh · 2021-07-20T17:26:47Z

+import {RibbonVault} from "./base/RibbonVault.sol";
+import {IRibbonThetaVault} from "../../interfaces/IRibbonThetaVault.sol";
+
+contract RibbonDeltaYearnVault is RibbonVault, OptionsDeltaYearnVaultStorage {


Since the Delta vaults don't deal with yvUSDC, why do we need to have a Yearn implementation?

I was thinking buying options whilst keeping the reserves in yvUSDC so your net loss every week is smaller. But I guess not worth.

kenchangh · 2021-07-20T17:27:38Z

+        address _oTokenFactory,
+        address _gammaController,
+        address _marginPool,
+        address _gnosisEasyAuction


We're missing passing the Yearn registry

in v1, we had it in constructor because the asset parameter was there. but the asset parameter is now in initialize, so I pass it in there. yearnRegistry is only used once to get the collateral token, so I don't think it is necessary to pass it in constructor, hold it as a constant, and then reference it in initialize. Just pass it into initialize

https://github.com/ribbon-finance/ribbon-v2/pull/47/files#diff-93cfb819847795b1c4f019da7b67280790d8b43a53485ac94b47b96d684b3e20R159

Understood, sounds good

kenchangh · 2021-07-20T17:51:18Z

+        }
+
+        if (vaultFee > 0) {
+            transferAsset(payable(feeRecipient), vaultFee);


Following this function here, just confirming that we're going to collect fees in yvUSDC?

I think it's fine to collect fees in yvUSDC btw, if it simplifies things

I changed it to be yvUSDC

kenchangh · 2021-07-20T17:58:18Z

+        require(newOption != address(0), "!nextOption");
+
+        // Wrap entire `asset` balance to `collateralToken` balance
+        _wrapToYieldToken();


Is it possible to move this to the bottom of the function? We're following the check-effect-interaction pattern so we should do interactions that involve asset transfers last.

kenchangh · 2021-07-20T17:58:34Z

+     */
+    function burnRemainingOTokens() external onlyOwner nonReentrant {
+        // Wrap entire `asset` balance to `collateralToken` balance
+        _wrapToYieldToken();


Same here https://github.com/ribbon-finance/ribbon-v2/pull/47/files#r673360238

kenchangh · 2021-07-21T18:26:05Z

-        returns (uint256 withdrawAmount)
-    {
+    function _withdrawYieldAndBaseToken(
+        address payable recipient,


We don't need payable since it's converted under the hood

chudnov · 2021-07-26T14:24:15Z

+    /*
+      Upgrades the vault to point to the latest yearn vault for the asset token
+    */
+    function upgradeYearnVault() external onlyOwner {


this should be called in between commitAndClose() and rollToNextOption(). only time we have all the collateral back in the vault

kenchangh · 2021-07-26T16:01:01Z

+            GAMMA_CONTROLLER,
+            MARGIN_POOL,
+            newOption,
+            wdiv(


We can try removing DSMath entirely as an import by copy pasting the wdiv/wmul functions into the contract, see if that helps with the size

kenchangh · 2021-07-26T16:02:52Z

+import {ShareMath} from "../../libraries/ShareMath.sol";
+import {RibbonVault} from "./base/RibbonVault.sol";
+
+contract RibbonThetaYearnVault is RibbonVault, OptionsThetaYearnVaultStorage {


We can remove OptionsThetaYearnVaultStorage since it's already inherited by RibbonVault?

good catch. originally made sense because we had theta / delta vaults so they diverged in whats needed for storage, but we have deleted delta

kenchangh · 2021-07-26T16:04:09Z

+    event NewOptionStrikeSelected(uint256 strikePrice, uint256 delta);
+
+    event PremiumDiscountSet(
+        uint256 premiumDiscount,
+        uint256 newPremiumDiscount
+    );
+
+    event AuctionDurationSet(
+        uint256 auctionDuration,
+        uint256 newAuctionDuration
+    );


We can remove these events imo

kenchangh · 2021-07-26T16:05:59Z

+import {ShareMath} from "../../libraries/ShareMath.sol";
 import {RibbonVault} from "./base/RibbonVault.sol";

 contract RibbonThetaVault is RibbonVault, OptionsThetaVaultStorage {


Same here https://github.com/ribbon-finance/ribbon-v2/pull/47/files#r676741657

we cannot do this because theta vaults inherit OptionsThetaVaultStorageV1 and delta vaults inherit OptionsDeltaVaultStorageV1.

both with different storage variables.

ribbon vault inherits OptionsVaultStorage which is the commonality between the two vaults

kenchangh · 2021-07-26T16:08:33Z

+        uint256 performanceFee,
+        uint256 vaultFee,
+        uint256 round


I must have missed this before, we should replace the vaultFee for managementFee

I actually added managementFee as well.

vaultFee is how much we actually made that week which is worth passing in

The dev can just do performanceFee + managementFee = vaultFee so it's redundant?

kenchangh · 2021-07-26T16:09:08Z

+                feeRecipient,
+                vaultFee
+            );
+            emit CollectVaultFees(performanceFee, vaultFee, vaultState.round);


Refer to https://github.com/ribbon-finance/ribbon-v2/pull/47/files#r676746116

emit CollectVaultFees(performanceFee, managementFee, vaultState.round);

…clean up pattern

kenchangh · 2021-08-02T14:30:09Z

+
+    before(async function () {
+      // Reset block
+      await network.provider.request({


Is there some way to avoid doing hardhat_reset? Calling reset breaks the test coverage tool.

Alternatively, we can use this solution to fork the state.

kenchangh · 2021-08-02T14:31:20Z

+        assert.isAtMost(receipt.gasUsed.toNumber(), 1045000);
+        // console.log("commitAndClose", receipt.gasUsed.toNumber());
+      });
+    });


Nit missing newline

kenchangh · 2021-08-02T14:54:46Z

+        premium = GnosisAuction.getOTokenPremium(
+            otokenAddress,
+            optionsPremiumPricer,
+            premiumDiscount
+        );


Needs to account for the fact that the put contracts are collateralized with yvUSDC

…asset than base

kenchangh · 2021-08-02T17:28:56Z

+        (uint256 lockedBalance, uint256 newPricePerShare, uint256 mintShares) =
+            VaultLifecycleYearn.rollover(
+                totalSupply(),
+                totalBalance(),


I think we're meant to pass in the collateralToken.balanceOf(address(this)) (yvUSDC balance) instead of totalBalance() (USDC balance)

This is likely why it's throwing off the calculation in VaultLifecycleYearn.createShort

…ce/ribbon-v2 into yearn-vault-upgrade

coveralls · 2021-08-03T15:56:21Z

Pull Request Test Coverage Report for Build 1094642468

0 of 384 (0.0%) changed or added relevant lines in 7 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-1.2%) to 1.608%

Changes Missing Coverage	Changed/Added Lines	%
contracts/vaults/BaseVaults/base/RibbonVault.sol	3	0.0%
contracts/vaults/BaseVaults/RibbonThetaVault.sol	4	0.0%
contracts/vaults/BaseVaults/RibbonDeltaVault.sol	5	0.0%
contracts/libraries/VaultLifecycle.sol	7	0.0%
contracts/vaults/YearnVaults/RibbonThetaYearnVault.sol	64	0.0%
contracts/vaults/YearnVaults/base/RibbonVault.sol	149	0.0%
contracts/libraries/VaultLifecycleYearn.sol	152	0.0%

Totals
Change from base Build 1078808900:	-1.2%
Covered Lines:	15
Relevant Lines:	858

💛 - Coveralls

kenchangh · 2021-08-03T16:50:07Z

    gasLimits: {
      depositWorstCase: 151608,
-      depositBestCase: 130594,
+      depositBestCase: 130681,


Follow up PR after this, let's gas golf the deposit flow for ThetaYearnVault

kenchangh reviewed Jul 20, 2021

View reviewed changes

kenchangh reviewed Jul 21, 2021

View reviewed changes

chudnov commented Jul 26, 2021

View reviewed changes

kenchangh reviewed Jul 26, 2021

View reviewed changes

Comment thread contracts/vaults/YearnVaults/base/RibbonVault.sol

kenchangh reviewed Jul 26, 2021

View reviewed changes

kenchangh force-pushed the yearn-vault-upgrade branch from de0e7c2 to b811b96 Compare July 29, 2021 13:23

John Doe and others added 14 commits July 29, 2021 14:48

update yearn vault to v2

2ca5f22

remove delta yearn vault, collect vault fees in yield bearing token, …

c7d0db3

…clean up pattern

start up test suite for theta vault yearn

1cc37c9

fix unit test syntax problems

520cd21

refactor yearn utility functions to vaultyearnlifecycle

47ee22f

yearn upgrade function

2ec0b15

fix lint

9589062

fix lint

49ddcaa

add test case for upgradeYearnVault

32a3427

address comments

1175be4

fix double commitAndClose lastlockedAmount issue

50c1395

change initial write to lastlockedamount

28f9ad2

fix error

e72e157

Rewrite updatePPS as a function

0d56f09

kenchangh added 3 commits July 29, 2021 16:40

Remove keepWrapped

a422555

Remove keepWrapped for completeWithdraw

d972e4e

Fix transfer USDC

34757eb

chudnov force-pushed the yearn-vault-upgrade branch 2 times, most recently from 7b4d3f8 to 34757eb Compare July 30, 2021 16:58

John Doe added 4 commits July 30, 2021 19:23

initial changes

f99b772

baseline realign with ribbonthetavault.ts

284e57a

fix totalBalance, most unit tests

74c2b3f

fix lint

49f99df

kenchangh reviewed Aug 2, 2021

View reviewed changes

John Doe and others added 6 commits August 2, 2021 17:02

multiply premium by pricePerShare as collateralized by more valuable …

d569fa2

…asset than base

add ioracle

02bc31e

fix upgradeYearnVault test

4558dd2

Check amountToWrap

c65a10a

check if there is something to wrap in first place

b829a5d

Remove console.logs

5f84ac8

kenchangh reviewed Aug 2, 2021

View reviewed changes

John Doe added 4 commits August 3, 2021 17:26

fix tests, misc fixes

bc4e384

Merge branch 'yearn-vault-upgrade' of https://github.com/ribbon-finan…

7a1740a

…ce/ribbon-v2 into yearn-vault-upgrade

fix lint

6d22e66

fix up base getVaultFees()

fc3fbfd

John Doe added 2 commits August 3, 2021 17:57

fix lastLockedAmount initial value

1156007

fix gas budget

1ebc56b

kenchangh reviewed Aug 3, 2021

View reviewed changes

kenchangh approved these changes Aug 3, 2021

View reviewed changes

chudnov merged commit ffc9fa2 into master Aug 3, 2021

chudnov deleted the yearn-vault-upgrade branch August 3, 2021 16:53

Conversation

chudnov commented Jul 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenchangh Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1094642468

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kenchangh Jul 21, 2021 •

edited

Loading

coveralls commented Aug 3, 2021 •

edited

Loading