chore: add comments on things that seem redundant or incorrect #32

igorline · 2022-05-31T16:44:43Z

No description provided.

packages/contracts/contracts/core/BaseVault.sol

Baraa42 · 2022-05-31T19:45:46Z

packages/contracts/contracts/core/HackMoneyVault.sol

@@ -23,6 +23,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {
    // Amount locked for scheduled withdrawals last week;
    uint128 public lastQueuedWithdrawAmount;
    // % of funds to be used for weekly option purchase
+    // TODO: Do we need this?


We dont need it, unless we introduce a limit on number of amount of options sold and have this to be upgradable. Idea is to start the system slowly and lift limitation with time

Right, but does not vault cap solve this?

Yes you are right, since we have one strategy per vault

packages/contracts/contracts/libraries/Vault.sol

Baraa42 · 2022-05-31T19:48:15Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -35,14 +35,17 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
        uint maxDeltaGap; // 5% ?
        uint minVol; // 80%
        uint maxVol; // 130%
+        // TODO: Do we need this?


we dont use maxVol, we can get rid of it

we also dont really use size, we can refactor the code

Baraa42 · 2022-05-31T19:50:31Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

        uint size; // 15
+        // TODO: This we don't use as of now. Maybe we should consider. TBC


We dont use it too, Im still reviewing my finance courses might have an input on that later

Baraa42 · 2022-05-31T19:50:55Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

        uint maxVolVariance; // 10%
        uint gwavPeriod;
    }

    HackMoneyStrategyDetail public strategyDetail;
    uint public activeExpiry;
    uint public currentBoardId;
+    // TODO: Should we move this into strategy details 


yes good point

Baraa42 · 2022-05-31T19:59:26Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -328,6 +333,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
        // update active strikes
        _addActiveStrike(strike.id, result.positionId);

+        // TODO: Remove this check as well?


I think this is redundant if we specify the minExpectedPremium inside the openPosition, the OptionMarket does _checkCostInBounds

Baraa42 · 2022-05-31T20:00:54Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -339,6 +345,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
    /**
     * @dev use premium in strategy to reduce position size if collateral ratio is out of range
     */
+    // TODO: Remove this


Is there any cases where we would want to reduce position? In the current definition of strategy Id say no, but would it be an "emergency" case where we would want to get out?

What is this "emergency" case? We might want to stop most of the actions and let users withdraw assets

yes that's something to think about later, maybe worth checking what's the security measures of Ribbon

Baraa42 · 2022-05-31T20:04:28Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -439,6 +446,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
     * @param collateralToLock collateral available to lock.
     * @return size how many options we can sell.
     */
+    // TODO: Never used. Remove?


this one was used for _getRequiredCollateral in case we trade different type of options. For the current strategy not useful we can drop

Baraa42 · 2022-05-31T20:06:24Z

packages/contracts/contracts/core/HackMoneyVault.sol

@@ -84,6 +85,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {
            // uint exchangeValue
        ) = strategy.doTrade(size);
        // update the remaining locked amount
+        // TODO: This might be fucked up as capitalUsed now returns total sum with premiums exchanged. I still need to have amount we have been able to get from exchange as I need to display this in the UI


Yes need correction, return one more variable

Baraa42 · 2022-05-31T20:07:52Z

packages/contracts/contracts/core/HackMoneyVault.sol

@@ -128,6 +131,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {

        strategy.setBoard(boardId);

+        // FIXME: Do we have tests for this?


Not really, once we have the script with the fork we can test the whole work flow for one week

Baraa42 · 2022-05-31T20:10:01Z

packages/contracts/contracts/core/HackMoneyVault.sol

@@ -138,6 +142,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {
        lastQueuedWithdrawAmount = uint128(queuedWithdrawAmount);
        emit RoundStarted(vaultState.round, uint104(lockedBalance));

+        // TODO: should we use lockedAmount?


they are the same at this moment, i dont see the difference?

Baraa42 · 2022-05-31T20:11:00Z

packages/contracts/contracts/core/HackMoneyVault.sol

@@ -151,6 +156,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {
    }

    // helper to set strategy size
+    // TODO: Do we need this?


do you need it for the UI? Could also need it for trading to know how much we still can trade

We can get it from subgraph easily

Baraa42 · 2022-05-31T20:12:29Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -459,6 +467,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
     * @dev verify if the strike is valid for the strategy
     * @return isValid true if vol is withint [minVol, maxVol] and delta is within targetDelta +- maxDeltaGap
     */
+    // TODO: Remove as never used or use in the code


Baraa42 · 2022-05-31T20:12:38Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -481,6 +490,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
    /**
     * @dev check if the vol variance for the given strike is within certain range
     */
+    // TODO: Remove as never used


Baraa42 · 2022-05-31T20:24:26Z

packages/contracts/contracts/strategies/HackMoneyStrategy.sol

@@ -505,6 +515,7 @@ contract HackMoneyStrategy is HackMoneyStrategyBase, IHackMoneyStrategy {
            secondsToExpiry <= strategyDetail.maxTimeToExpiry);
    }

+    // TODO: Think how to handle dynamic fees of synthetix not to sell cheap.. https://blog.synthetix.io/dynamic-exchange-fees-explained/


I dont think this is an issue, we use Synthetix to exchange premiums. Premiums are small portion of the notional and fees on exchanging premiums is a small portion of premiums, a max of 0.3% rare difference on premiums will not hurt in my opinion. Besides we are trading eth/usd which is not subject to high volatility compared to other markets. However Ill check, maybe we set a max fee to accept say 0.25%?

Maybe we can have max slippage specified in the contract

Synthetix have 0 slippage, see https://docs.synthetix.io/litepaper/#system-architecture (cmd+F 'slippage'), only trading fees are paid.

Yep, but that's what I refer. We can have slippage that would be reflecting fees that we have paid

Baraa42

Commented on your notes, let me know when you open tickets

chore: add comments on things that seem redundant or incorrect

0f1f942

igorline requested a review from Baraa42 May 31, 2022 16:44

Baraa42 reviewed May 31, 2022

View reviewed changes

packages/contracts/contracts/core/BaseVault.sol Show resolved Hide resolved

Baraa42 reviewed May 31, 2022

View reviewed changes

packages/contracts/contracts/libraries/Vault.sol Show resolved Hide resolved

Baraa42 reviewed May 31, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add comments on things that seem redundant or incorrect #32

chore: add comments on things that seem redundant or incorrect #32

igorline commented May 31, 2022

Baraa42 May 31, 2022

igorline May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

igorline May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

igorline May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

Baraa42 May 31, 2022

igorline May 31, 2022

Baraa42 May 31, 2022

igorline Jun 1, 2022

Baraa42 left a comment

		uint size; // 15
		// TODO: This we don't use as of now. Maybe we should consider. TBC

		@@ -128,6 +131,7 @@ contract HackMoneyVault is Multicall, Ownable, BaseVault {

		strategy.setBoard(boardId);

		// FIXME: Do we have tests for this?

chore: add comments on things that seem redundant or incorrect #32

Are you sure you want to change the base?

chore: add comments on things that seem redundant or incorrect #32

Conversation

igorline commented May 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baraa42 left a comment

Choose a reason for hiding this comment