PCV Deposit V2 #181

eswak · 2023-01-23T11:06:32Z

Update comment

eswak · 2023-01-23T11:14:37Z

Why the duplication ?

eswak · 2023-01-23T11:18:31Z

I don't think this logic of reward token should be at the PCV Deposit level.

The compound contracts are effectively airdropped COMP tokens, but it may not be the case in the future.

Also, other airdrops/rewards happen, such as the morpho deposit that will earn both COMP and MORPHO. Or an hypothetical protocol launching and performs an airdrop to Aave/Euler users, etc. This logic of reward handling should be isolated from the PCVDeposit logic.

Morpho deposit requires that you call into contracts to claim the rewards from the depositor. I think it makes sense to leave these methods on the generic contract and then only implement logic if necessary on the deposits.

eswak · 2023-01-23T11:15:21Z

Update comment

eswak · 2023-01-23T11:15:04Z

Update comment

eswak · 2023-01-23T11:21:11Z

I don't think this should be handled at the PCVDeposit level, because if a PCVDeposit implementation cannot properly detect an insolvency in the underlying venue, it will report an erroneous balance to the PCVOracle. The PCVOracle can fix this by changing the venue's oracle price feed (and properly account the loss thanks to the oracle update). But if the accumulative profit logic is handled by the PCVDeposit, it will also report erroneous accumulative profits, and these won't be able to be corrected by updating the oracle feed in PCVOracle.

See PR #193 for updated logic around profit tracking on PCV Deposits. All logic has now been moved to the PCV Oracle.

eswak · 2023-01-23T11:34:32Z

from my experience, it's almost impossible to build and hardcode the liquidity reward claiming payload onchain. Some of them are completely handled offchain altogether, or will migrate their distribution contracts, or will build use merkle trees that require offchain proof reading, or will use layer 2s, or will need multiple different contract calls at different times (so there should be a harvest and a requestHarvest function for instance), etc...

it's better to separate this logic from the PCVDeposit logic completely and have a separate architecture that is able to do things like : "Oh, this contract has COMP tokens airdropped on it ? I'll pull these, sell them for DAI, and airdrop back the DAI to the contract". We could have a separate "depositProfit" method if we want these proceedings to be properly accounted as profit, but the PCVDeposit logic itself should be as simple as possible, and only handle the balanceReportedIn token + venue entry/exit

I agree that there is lots of ways to handle these reward tokens, however, certain implementations like Morpho require the msg.sender be the recipient of the tokens. If other deposits do not require this, we can just leave this _claim() logic empty.

eswak · 2023-01-23T11:41:56Z

update comment

eswak · 2023-01-23T15:20:07Z

Add a test that calls this, it's not covered

eswak · 2023-01-23T11:42:54Z

you can avoid a sload here

eswak · 2023-01-23T15:20:30Z

Add a test that calls this, it's not covered

eswak · 2023-01-23T11:46:31Z

update comment

eswak · 2023-01-23T11:48:56Z

can we have shorter names that are coherent with each other ? like _enter() _exit() _accrue()

eswak · 2023-01-23T11:51:36Z

Why change ? I liked having external contracts/interfaces in a separate folder. It's also convenient for code coverage report (doesn't matter for interfaces, but it matters for contracts), and we need consistent code coverage reports if we want it to be part of CI checks someday. I'd find it weird to have external interfaces all over the place, and external contracts in another folder.

Also, why put the Compound interface in the morpho folder, and not in the Compound folder ?

-Original file line number
+Diff line change
@@ Expand Up / @@ -216,7 +216,7 @@ contract ERC20Allocator is IERC20Allocator, CoreRefV2 { @@
             (uint256 amountToDrip, uint256 adjustedAmountToDrip) = getDripDetails(
                 psm,
-                pcvDeposit
+                address(pcvDeposit)
             );
             /// Effects
@@ Expand Down Expand Up / @@ -299,7 +299,7 @@ contract ERC20Allocator is IERC20Allocator, CoreRefV2 { @@
         /// reverts if not drip eligbile
         function getDripDetails(
             address psm,
-            PCVDeposit pcvDeposit
+            address pcvDeposit
         ) public view returns (uint256 amountToDrip, uint256 adjustedAmountToDrip) {
             PSMInfo memory toDrip = allPSMs[psm];
@@ Expand All / @@ -324,7 +324,7 @@ contract ERC20Allocator is IERC20Allocator, CoreRefV2 { @@
             /// amountToDrip = 1,000e6
             amountToDrip = Math.min(
-                Math.min(targetBalanceDelta, pcvDeposit.balance()),
+                Math.min(targetBalanceDelta, PCVDeposit(pcvDeposit).balance()),
                 /// adjust for decimals here as buffer is 1e18 scaled,
                 /// and if token is not scaled by 1e18, then this amountToDrip could be over the buffer
                 /// because buffer is 1e18 adjusted, and decimals normalizer is used to adjust up to the buffer
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -1,14 +1,83 @@
     // SPDX-License-Identifier: GPL-3.0-or-later
     pragma solidity 0.8.13;
-    import {IPCVDeposit} from "@voltprotocol/pcv/IPCVDeposit.sol";
     /// @title PCV V2 Deposit interface
     /// @author Volt Protocol
-    interface IPCVDepositV2 is IPCVDeposit {
-        // ----------- State changing api -----------
+    interface IPCVDepositV2 {
+        // ----------- Events -----------
+        event Deposit(address indexed _from, uint256 _amount);
+        event Withdrawal(
+            address indexed _caller,
+            address indexed _to,
+            uint256 _amount
+        );
+        event WithdrawERC20(
+            address indexed _caller,
+            address indexed _token,
+            address indexed _to,
+            uint256 _amount
+        );
+        event WithdrawETH(
+            address indexed _caller,
+            address indexed _to,
+            uint256 _amount
+        );
+        event Harvest(address indexed _token, int256 _profit, uint256 _timestamp);
+        // ----------- PCV Controller only state changing api -----------
+        /// @notice withdraw tokens from the PCV allocation
+        /// non-reentrant as state changes and external calls are made
+        /// @param to the address PCV will be sent to
+        /// @param amount of tokens withdrawn
+        function withdraw(address to, uint256 amount) external;
+        /// @notice withdraw ERC20 from the contract
+        /// @param token address of the ERC20 to send
+        /// @param to address destination of the ERC20
+        /// @param amount quantity of ERC20 to send
+        /// Calling this function will lead to incorrect
+        /// accounting in a PCV deposit that tracks
+        /// profits and or last recorded balance.
+        /// If a deposit records PNL, only use this
+        /// function in an emergency.
+        function withdrawERC20(address token, address to, uint256 amount) external;
+        // ----------- Permissionless State changing api -----------
+        /// @notice deposit ERC-20 tokens to underlying venue
+        /// non-reentrant to block malicious reentrant state changes
+        /// to the lastRecordedBalance variable
+        function deposit() external;
+        /// @notice claim COMP rewards for supplying to Morpho.
+        /// Does not require reentrancy lock as no smart contract state is mutated
+        /// in this function.
         function harvest() external;
+        /// @notice function that emits an event tracking profits and losses
+        /// since the last contract interaction
+        /// then writes the current amount of PCV tracked in this contract
+        /// to lastRecordedBalance
+        /// @return the amount deposited after adding accrued interest or realizing losses
         function accrue() external returns (uint256);
+        // ----------- Getters -----------
+        /// @notice gets the effective balance of "balanceReportedIn" token if the deposit were fully withdrawn
+        function balance() external view returns (uint256);
+        /// @notice gets the token address in which this deposit returns its balance
+        function balanceReportedIn() external view returns (address);
+        /// @notice address of underlying token
+        function token() external view returns (address);
+        /// @notice address of reward token
+        function rewardToken() external view returns (address);
     }

-Original file line number
+Diff line change
@@ Expand Up @@
     import {CoreRefV2} from "@voltprotocol/refs/CoreRefV2.sol";
     import {PCVDeposit} from "@voltprotocol/pcv/PCVDeposit.sol";
     import {PCVGuardian} from "@voltprotocol/pcv/PCVGuardian.sol";
-    import {IComptroller} from "@voltprotocol/external/compound/ICompound.sol";
+    import {IComptroller} from "@voltprotocol/pcv/morpho/ICompound.sol";
     import {ICompoundBadDebtSentinel} from "@voltprotocol/pcv/compound/ICompoundBadDebtSentinel.sol";
     /// @notice Contract that removes all funds from Compound
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,275 @@
+    // SPDX-License-Identifier: GPL-3.0-or-later
+    pragma solidity 0.8.13;
+    import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+    import {SafeCast} from "@openzeppelin/contracts/utils/math/SafeCast.sol";
+    import {Address} from "@openzeppelin/contracts/utils/Address.sol";
+    import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+    import {ILens} from "@voltprotocol/pcv/morpho/ILens.sol";
+    import {ICToken} from "@voltprotocol/pcv/morpho/ICompound.sol";
+    import {IMorpho} from "@voltprotocol/pcv/morpho/IMorpho.sol";
+    import {CoreRefV2} from "@voltprotocol/refs/CoreRefV2.sol";
+    import {IPCVDepositV2} from "@voltprotocol/pcv/IPCVDepositV2.sol";
+    /// @title abstract contract for withdrawing ERC-20 tokens using a PCV Controller
+    /// @author Elliot Friedman
+    abstract contract PCVDepositV2 is IPCVDepositV2, CoreRefV2 {
+        using SafeERC20 for IERC20;
+        using SafeCast for *;
+        /// @notice reference to underlying token
+        address public immutable override rewardToken;
+        /// @notice reference to underlying token
+        address public immutable override token;
+        /// ------------------------------------------
+        /// ------------- State Variables ------------
+        /// ------------------------------------------
+        /// @notice track the last amount of PCV recorded in the contract
+        /// this is always out of date, except when accrue() is called
+        /// in the same block or transaction. This means the value is stale
+        /// most of the time.
+        uint128 public lastRecordedBalance;
+        /// @notice track the last amount of profits earned by the contract
+        /// this is always out of date, except when accrue() is called
+        /// in the same block or transaction. This means the value is stale
+        /// most of the time.
+        int128 public lastRecordedProfits;
+        constructor(address _token, address _rewardToken) {
+            token = _token;
+            rewardToken = _rewardToken;
+        }
+        /// ------------------------------------------
+        /// -------------- View Only API -------------
+        /// ------------------------------------------
+        /// @notice return the underlying token denomination for this deposit
+        function balanceReportedIn() external view returns (address) {
+            return address(token);
+        }
+        /// ------------------------------------------
+        /// ----------- Permissionless API -----------
+        /// ------------------------------------------
+        /// @notice deposit ERC-20 tokens to Morpho-Compound
+        /// non-reentrant to block malicious reentrant state changes
+        /// to the lastRecordedBalance variable
+        function deposit() public whenNotPaused globalLock(2) {
+            /// ------ Check ------
+            uint256 amount = IERC20(token).balanceOf(address(this));
+            if (amount == 0) {
+                /// no op to prevent revert on empty deposit
+                return;
+            }
+            int256 startingRecordedBalance = lastRecordedBalance.toInt256();
+            /// ------ Effects ------
+            /// compute profit from interest accrued and emit an event
+            /// if any profits or losses are realized
+            int256 profit = _recordPNL();
+            /// increment tracked recorded amount
+            /// this will be off by a hair, after a single block
+            /// negative delta turns to positive delta (assuming no loss).
+            lastRecordedBalance += uint128(amount);
+            /// ------ Interactions ------
+            /// approval and deposit into underlying venue
+            _supply(amount);
+            /// ------ Update Internal Accounting ------
+            int256 endingRecordedBalance = balance().toInt256();
+            _pcvOracleHook(endingRecordedBalance - startingRecordedBalance, profit);
+            emit Deposit(msg.sender, amount);
+        }
+        /// @notice claim COMP rewards for supplying to Morpho.
+        /// Does not require reentrancy lock as no smart contract state is mutated
+        /// in this function.
+        function harvest() external globalLock(2) {
+            uint256 claimedAmount = _claim();
+            emit Harvest(rewardToken, int256(claimedAmount), block.timestamp);
+        }
+        /// @notice function that emits an event tracking profits and losses
+        /// since the last contract interaction
+        /// then writes the current amount of PCV tracked in this contract
+        /// to lastRecordedBalance
+        /// @return the amount deposited after adding accrued interest or realizing losses
+        function accrue() external globalLock(2) whenNotPaused returns (uint256) {
+            int256 profit = _recordPNL(); /// update deposit amount and fire harvest event
+            /// if any amount of PCV is withdrawn and no gains, delta is negative
+            _pcvOracleHook(profit, profit);
+            return lastRecordedBalance; /// return updated pcv amount
+        }
+        /// ------------------------------------------
+        /// ------------ Permissioned API ------------
+        /// ------------------------------------------
+        /// @notice withdraw tokens from the PCV allocation
+        /// non-reentrant as state changes and external calls are made
+        /// @param to the address PCV will be sent to
+        /// @param amount of tokens withdrawn
+        function withdraw(
+            address to,
+            uint256 amount
+        ) external onlyPCVController globalLock(2) {
+            int256 profit = _withdraw(to, amount, true);
+            /// if any amount of PCV is withdrawn and no gains, delta is negative
+            _pcvOracleHook(-(amount.toInt256()) + profit, profit);
+        }
+        /// @notice withdraw all tokens from Morpho
+        /// non-reentrant as state changes and external calls are made
+        /// @param to the address PCV will be sent to
+        function withdrawAll(address to) external onlyPCVController globalLock(2) {
+            /// compute profit from interest accrued and emit an event
+            int256 profit = _recordPNL();
+            int256 recordedBalance = lastRecordedBalance.toInt256();
+            /// withdraw last recorded amount as this was updated in record pnl
+            _withdraw(to, lastRecordedBalance, false);
+            /// all PCV withdrawn, send call in with amount withdrawn negative if any amount is withdrawn
+            _pcvOracleHook(-recordedBalance, profit);
+        }
+        /// @notice withdraw ERC20 from the contract
+        /// @param tokenAddress address of the ERC20 to send
+        /// @param to address destination of the ERC20
+        /// @param amount quantity of ERC20 to send
+        /// Calling this function will lead to incorrect
+        /// accounting in a PCV deposit that tracks
+        /// profits and or last recorded balance.
+        /// If a deposit records PNL, only use this
+        /// function in an emergency.
+        function withdrawERC20(
+            address tokenAddress,
+            address to,
+            uint256 amount
+        ) public virtual override onlyPCVController {
+            IERC20(tokenAddress).safeTransfer(to, amount);
+            emit WithdrawERC20(msg.sender, tokenAddress, to, amount);
+        }
+        /// ------------- Internal Helpers -------------
+        /// @notice records how much profit or loss has been accrued
+        /// since the last call and emits an event with all profit or loss received.
+        /// Updates the lastRecordedBalance to include all realized profits or losses.
+        /// @return profit accumulated since last _recordPNL() call.
+        function _recordPNL() internal returns (int256) {
+            /// first accrue interest in the underlying venue
+            _accrueUnderlying();
+            /// ------ Check ------
+            /// then get the current balance from the market
+            uint256 currentBalance = balance();
+            /// save gas if contract has no balance
+            /// if cost basis is 0 and last recorded balance is 0
+            /// there is no profit or loss to record and no reason
+            /// to update lastRecordedBalance
+            if (currentBalance == 0 && lastRecordedBalance == 0) {
+                return 0;
+            }
+            /// currentBalance should always be greater than or equal to
+            /// the deposited amount, except on the same block a deposit occurs, or a loss event in morpho
+            /// SLOAD
+            uint128 _lastRecordedBalance = lastRecordedBalance;
+            /// Compute profit
+            int128 profit = int128(int256(currentBalance)) -
+                int128(_lastRecordedBalance);
+            int128 _lastRecordedProfits = lastRecordedProfits + profit;
+            /// ------ Effects ------
+            /// SSTORE: record new amounts
+            lastRecordedProfits = _lastRecordedProfits;
+            lastRecordedBalance = uint128(currentBalance);
+            /// profit is in underlying token
+            emit Harvest(token, int256(profit), block.timestamp);
+            return profit;
+        }
+        /// @notice helper avoid repeated code in withdraw and withdrawAll
+        /// anytime this function is called it is by an external function in this smart contract
+        /// with a reentrancy guard. This ensures lastRecordedBalance never desynchronizes.
+        /// Morpho is assumed to be a loss-less venue. over the course of less than 1 block,
+        /// it is possible to lose funds. However, after 1 block, deposits are expected to always
+        /// be in profit at least with current interest rates around 0.8% natively on Compound,
+        /// ignoring all COMP and Morpho rewards.
+        /// @param to recipient of withdraw funds
+        /// @param amount to withdraw
+        /// @param recordPnl whether or not to record PnL. Set to false in withdrawAll
+        /// as the function _recordPNL() is already called before _withdraw
+        function _withdraw(
+            address to,
+            uint256 amount,
+            bool recordPnl
+        ) private returns (int256 profit) {
+            /// ------ Effects ------
+            if (recordPnl) {
+                /// compute profit from interest accrued and emit a Harvest event
+                profit = _recordPNL();
+            }
+            /// update last recorded balance amount
+            /// if more than is owned is withdrawn, this line will revert
+            /// this line of code is both a check, and an effect
+            lastRecordedBalance -= uint128(amount);
+            /// ------ Interactions ------
+            /// remove funds from underlying venue
+            _withdrawAndTransfer(amount, to);
+            emit Withdrawal(msg.sender, to, amount);
+        }
+        /// ------------- Virtual Functions -------------
+        /// @notice get balance in the underlying market.
+        /// @return current balance of deposit
+        function balance() public view virtual override returns (uint256);
+        /// @dev accrue interest in the underlying market.
+        function _accrueUnderlying() internal virtual;
+        /// @dev withdraw from the underlying market.
+        function _withdrawAndTransfer(uint256 amount, address to) internal virtual;
+        /// @dev deposit in the underlying market.
+        function _supply(uint256 amount) internal virtual;
+        /// @dev claim rewards from the underlying market.
+        /// returns amount of reward tokens claimed
+        function _claim() internal virtual returns (uint256);
+    }

PCV Deposit V2 #181

Are you sure you want to change the base?

Uh oh!

PCV Deposit V2 #181

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!