Rocket Pool Vault #18

ez7212 · 2022-04-23T19:56:06Z

added a "user" parameter to depositLpToken to call transferFrom

pending issues:

RocketPool deposit limit is 2k ETH in pool
RocketPool doesn't allow withdrawals unless there is ETH available in the pool
should we add these checks in RocketPoolVault or VaultAccounting?

ez7212 · 2022-04-26T16:34:47Z

i might've put the interfaces into the wrong folder so lmk if there's a better location

Oozyx

I would also create a new folder in interfaces. call it vaults or something and put the interfaces related to vaults in there.

Oozyx · 2022-04-28T11:20:29Z

contracts/interfaces/RocketDepositPoolInterface.sol

@@ -0,0 +1,15 @@
+pragma solidity 0.8.0;
+
+// SPDX-License-Identifier: GPL-3.0-only


Goes above pragma solidity

Oozyx · 2022-04-28T11:21:51Z

contracts/interfaces/RocketDepositPoolInterface.sol

+
+interface RocketDepositPoolInterface {
+    function getBalance() external view returns (uint256);
+    function getExcessBalance() external view returns (uint256);


New line in between each function. Keep same style as other interface files.

Oozyx · 2022-04-28T11:22:21Z

contracts/interfaces/RocketTokenRETHInterface.sol

@@ -0,0 +1,17 @@
+pragma solidity 0.8.0;
+
+// SPDX-License-Identifier: GPL-3.0-only


Above pragma solidity

Oozyx · 2022-04-28T11:22:30Z

contracts/interfaces/RocketTokenRETHInterface.sol

+
+interface RocketTokenRETHInterface is IERC20 {
+    function getEthValue(uint256 _rethAmount) external view returns (uint256);
+    function getRethValue(uint256 _ethAmount) external view returns (uint256);


Oozyx · 2022-04-28T11:25:56Z

contracts/vaults/RocketPoolVault.sol

+
+contract RocketVault is BaseVault {
+
+    RocketDepositPoolInterface depositPool;


Add comments to explain what these variables are

Oozyx · 2022-04-28T14:01:55Z

contracts/vaults/RocketPoolVault.sol

+    RocketTokenRETHInterface rETH;
+
+    constructor (address _diamond, address _rETHAddress, address _depositPool) BaseVault(_diamond) {
+        depositPool = RocketDepositPoolInterface(_depositPool);


After looking at the docs, you should only store the RocketStorageInterface address, and use it to get the current addresses of the rocketdepositpool and rockettokenreth

Oozyx · 2022-04-28T14:07:52Z

contracts/vaults/RocketPoolVault.sol

+    }
+
+    function getAmountETH(uint256 lpTokenAmount) external view override returns (uint256) {
+        return rETH.getEthValue(lpTokenAmount);


There may be a limit to how much could be withdrawn. I think to avoid having bids that can't be covered (not enough excess ETH in the pool to withdraw), we should make a check that takes this in consideration

added
should we also add a check for depositing (if the rocket vault is not accepting more ETH) and 24 hr check after depositing?
I believe rocket already has these checks in place, but wondering if we need them on our side too

Don't think we need to add checks for depositing. If it does revert I don't think it's a big deal. Mainly on the withdraw we want to avoid reverting.

I don't see the check for the withdraw limit?

Oozyx · 2022-05-02T13:14:47Z

contracts/interfaces/vaults/RocketDepositPoolInterface.sol

@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-3.0-only
+pragma solidity 0.8.0;


wrong solidity version (should be ^0.8.0)

Oozyx · 2022-05-02T13:15:06Z

contracts/interfaces/vaults/RocketStorageInterface.sol

+
+interface RocketStorageInterface {
+
+    // Getters


remove comment

Oozyx · 2022-05-02T13:15:15Z

contracts/interfaces/vaults/RocketStorageInterface.sol

@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: GPL-3.0-only
+pragma solidity 0.8.0;


wrong solidity version

Oozyx · 2022-05-02T13:15:26Z

contracts/interfaces/vaults/RocketTokenRETHInterface.sol

@@ -0,0 +1,14 @@
+// SPDX-License-Identifier: GPL-3.0-only
+pragma solidity 0.8.0;


wrong solidity version

Oozyx · 2022-05-02T13:25:28Z

contracts/vaults/RocketPoolVault.sol

+    }
+
+    function getAmountETH(uint256 lpTokenAmount) external view override returns (uint256) {
+        return rETH.getEthValue(lpTokenAmount);


Don't think we need to add checks for depositing. If it does revert I don't think it's a big deal. Mainly on the withdraw we want to avoid reverting.

Oozyx · 2022-05-02T13:26:43Z

contracts/vaults/RocketPoolVault.sol

+    }
+
+    function getAmountETH(uint256 lpTokenAmount) external view override returns (uint256) {
+        return rETH.getEthValue(lpTokenAmount);


I don't see the check for the withdraw limit?

Oozyx · 2022-05-02T13:27:35Z

contracts/vaults/RocketPoolVault.sol

+    }
+
+    function getAmountETH(uint256 lpTokenAmount) external view override returns (uint256) {
+        return rETH.getEthValue(lpTokenAmount);


Oozyx · 2022-05-02T13:29:40Z

contracts/vaults/RocketPoolVault.sol

+        address rethAddress = rocketStorage.getAddress(keccak256(abi.encodePacked("contract.address", "rocketETHToken")));
+        RocketTokenRETHInterface rETH = RocketTokenRETHInterface(rethAddress);
+
+        // check that rocketVault has enough ETH to handle withdrawal


I think this check should be in the getAmountEth function. Reason is because we use this function when checking if a bid is valid (enough eth in vaults to cover bid). With this change we will not be displaying bids if rocket pool limits the withdrawal amount

Yea makes sense. I'll move the check to getAmountEth

The withdraw function in vaultAccountingFacet doesn't call getAmountEth though. If a user wants to withdraw from the vault, I'm guessing its ok that it reverts too? Since we only want to avoid reverting when a bid is actually accepted

Moved the check to getAmountEth. I currently have it as an if statement where if rocket vault doesn't have enough eth to match the eth amount result, it returns the rocket vault balance. This may mess up the userETHBalance function in VaultAccountingFacet though.

If I have a require(rocketVault.balance >= ethAmount, "not enough ETH to withdraw"), I'm assuming it would revert sometimes. Wondering what you think is the best way to go about this

The userETHBalance function calls the vault's getAmountETH function, so this should be good.

Oozyx · 2022-05-02T13:30:49Z

Also need to add tests for this. Would probably be similar to how we test the vaults in the old repo. Mainnet fork for the rocket pool testing. I'll get the testing started with the empty vault.

Oozyx · 2022-05-03T10:50:16Z

contracts/vaults/RocketPoolVault.sol

+        uint256 ethAmount = rETH.getEthValue(lpTokenAmount);
+
+        // check if rocket vault has enough to withdraw, if not return rocket vault balance 
+        if (rocketVault.balance >= ethAmount) {


Should call the contract's balanceOf function. https://github.com/rocket-pool/rocketpool/blob/master/contracts/contract/RocketVault.sol#L39

Oozyx · 2022-05-03T10:52:17Z

contracts/vaults/RocketPoolVault.sol

+        address rethAddress = rocketStorage.getAddress(keccak256(abi.encodePacked("contract.address", "rocketETHToken")));
+        RocketTokenRETHInterface rETH = RocketTokenRETHInterface(rethAddress);
+
+        // check that rocketVault has enough ETH to handle withdrawal


The userETHBalance function calls the vault's getAmountETH function, so this should be good.

Oozyx · 2022-05-03T12:12:07Z

contracts/interfaces/vaults/IVault.sol

@@ -12,9 +12,10 @@ interface IVault {
    /**
     * @dev Deposits LP token directly into vault
     *
+     * @param user The user depositing 


params are not in order

Oozyx · 2022-05-04T11:08:27Z

Also you didn't run prettier. You should set it up so it automatically applies when saving.
Here are the settings I use in VSCode:
"editor.formatOnSave": true,
"editor.defaultFormatter": "esbenp.prettier-vscode",
"editor.rulers": [120],
"editor.tabSize": 2,

…ukuNFT/fuku-nft-diamond into feat/rocket-delegate-mechanism

Rocket Pool Delegate

Oozyx · 2022-05-04T08:42:59Z

contracts/interfaces/vaults/RocketVaultInterface.sol

@@ -0,0 +1,16 @@
+pragma solidity ^0.8.0;
+
+// SPDX-License-Identifier: GPL-3.0-only


Goes above pragma solidity

Oozyx · 2022-05-04T08:43:15Z

contracts/interfaces/vaults/RocketVaultInterface.sol

+pragma solidity ^0.8.0;
+
+// SPDX-License-Identifier: GPL-3.0-only
+import "@openzeppelin/contracts/token/ERC20/IERC20.sol";


Import {IERC20} from ...

Oozyx · 2022-05-04T08:43:28Z

contracts/interfaces/vaults/RocketVaultInterface.sol

+import "@openzeppelin/contracts/token/ERC20/extensions/ERC20Burnable.sol";
+
+interface RocketVaultInterface {
+    function balanceOf(string memory _networkContractName) external view returns (uint256);


New lines in between each function

Oozyx · 2022-05-04T08:44:21Z

hardhat.config.js

+    hardhat: {
+      forking: {
+        url: MAINNET_URL,
+        enabled: true,


Make this false by default

I merged main so this should be fixed

Oozyx · 2022-05-05T10:40:33Z

hardhat.config.js


 module.exports = {
  networks: {
    hardhat: {
      forking: {
        url: MAINNET_URL,
-        enabled: true,
+        // enabled: true,


Don't comment out, set enabled to false

Oozyx · 2022-05-25T14:25:47Z

contracts/facets/BidMarketFacet.sol

@@ -152,7 +152,7 @@ contract BidMarketFacet is IBidMarket {
        // update user balance
        vs.userVaultBalances[bidInfo.bidder][bidInfo.bidInput.vault] -= bidLPTokenAmount;
        // withdraw funds from vault
-        uint256 ethReturned = vault.withdraw(bidLPTokenAmount, payable(this));
+        uint256 ethReturned = vault.withdraw(bidLPTokenAmount, payable(this), abi.encode(bidInfo.bidder));


use vaultlibutils for optional data

we're encoding the bidder's address instead of msg.sender here. how would I go about adding this to the vaultLibUtils?

Oozyx · 2022-05-25T14:25:55Z

contracts/facets/OptionMarketFacet.sol

@@ -156,7 +156,7 @@ contract OptionMarketFacet is IOptionMarket, IERC721Receiver {
        // update user balance
        vs.userVaultBalances[option.bidder][option.optionInput.bidInput.vault] -= premiumLPTokenAmount;
        // withdraw the premium from bidder's vault
-        uint256 ethReturned = vault.withdraw(premiumLPTokenAmount, payable(this));
+        uint256 ethReturned = vault.withdraw(premiumLPTokenAmount, payable(this), abi.encode(option.bidder));


use vaultlibutils for optional data

Oozyx · 2022-05-25T14:26:00Z

contracts/facets/OptionMarketFacet.sol

@@ -244,7 +244,11 @@ contract OptionMarketFacet is IOptionMarket, IERC721Receiver {
        // update user balance
        vs.userVaultBalances[option.bidder][option.optionInput.bidInput.vault] -= strikeLPTokenAmount;
        // withdraw the strike amount from bidder's vault
-        uint256 ethReturned = vault.withdraw(strikeLPTokenAmount, payable(oms.acceptedOptions[optionId].seller));
+        uint256 ethReturned = vault.withdraw(


use vaultlibutils for optional data

ez7212 added 6 commits April 23, 2022 14:45

add msg.sender to depositLpToken

4cd4e20

add user param to depositLpToken

3f849f7

add address param

f0b50e4

initial commit

6a18b22

initial commit

2e9152e

inital commit

6edefca

Oozyx reviewed Apr 28, 2022

View reviewed changes

Oozyx changed the base branch from feat/tokenomics to main April 28, 2022 13:45

Oozyx reviewed Apr 28, 2022

View reviewed changes

ez7212 added 3 commits April 28, 2022 11:45

fix IVault directory

d676eea

new directory for vault interfaces

f608078

import only RocketStorage and add withdrawal check

f6d1280

Oozyx reviewed May 2, 2022

View reviewed changes

ez7212 added 3 commits May 2, 2022 22:22

update solidity version

210d35f

update solidity version and remove comment

962ae49

move withdraw check to getAmountEth

7543bac

Oozyx reviewed May 3, 2022

View reviewed changes

ez7212 added 7 commits May 3, 2022 17:07

rearrange params

87920b3

change vault balance getter function

2f39bbd

initial commit

4cca37b

add forking config

609ce9a

add rocket vault deployment

03bd301

rocket test initial commit

dfa61c0

fix comment

ba2a92d

ez7212 and others added 29 commits May 17, 2022 16:38

fix imports, param order, and add vault storage creation

2c7d253

remove initialize and ownable

de0a184

add comment

da6db33

add address encoding

0934d39

change bytes32 param to address

51ca073

change bytes32 to bytes

cbcbb49

add bytes param

756c841

add bytes param

5c22445

add receive function

d86864f

add bytes decoder and getter for current implementation

1125200

import interface and change bytes to address

49dcc9b

add userData for tests to pass

538d306

add userData for tests to pass

059e8c7

add delegate testing

81fee31

more tests

1ea26d2

Refactor suggestions

3ba4746

remove unneeded functions

418e5fb

change implementation functions to ownership

d8ce179

Merge branch 'feat/rocket-delegate-mechanism' of https://github.com/F…

090ec87

…ukuNFT/fuku-nft-diamond into feat/rocket-delegate-mechanism

add getVaultOptionalData helper

d4bac30

multiple user withdraw tests

953ab23

minor changes

2da5cac

rocket vault without creating new vault storage

9d25759

upgrade rocket vault and ensure withdrawals work

d1d78a9

Use fixture constant

0c89801

change impersonate deposit amt

0ca165f

Use RocketPoolVaultStorage interface

c383576

Remove text file

beb594b

Merge pull request #26 from FukuNFT/feat/rocket-delegate-mechanism

82ea2e4

Rocket Pool Delegate

Oozyx reviewed May 25, 2022

View reviewed changes

		@@ -0,0 +1,15 @@
		pragma solidity 0.8.0;

		// SPDX-License-Identifier: GPL-3.0-only

		@@ -0,0 +1,17 @@
		pragma solidity 0.8.0;

		// SPDX-License-Identifier: GPL-3.0-only


		contract RocketVault is BaseVault {

		RocketDepositPoolInterface depositPool;

		@@ -0,0 +1,9 @@
		// SPDX-License-Identifier: GPL-3.0-only
		pragma solidity 0.8.0;

		@@ -0,0 +1,14 @@
		// SPDX-License-Identifier: GPL-3.0-only
		pragma solidity 0.8.0;

		@@ -0,0 +1,16 @@
		pragma solidity ^0.8.0;

		// SPDX-License-Identifier: GPL-3.0-only

Rocket Pool Vault #18

Are you sure you want to change the base?

Rocket Pool Vault #18

Conversation

ez7212 commented Apr 23, 2022

ez7212 commented Apr 26, 2022

Oozyx left a 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

ez7212 May 2, 2022 • edited Loading

Choose a reason for hiding this comment

ez7212 May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oozyx commented May 2, 2022 • edited Loading

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

Oozyx commented May 4, 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

ez7212 May 2, 2022 •

edited

Loading

ez7212 May 3, 2022 •

edited

Loading

Oozyx commented May 2, 2022 •

edited

Loading