Add Borrowable Margin Pool (V2) #459

chudnov · 2022-07-19T11:14:05Z

Task: Feature Name:

High Level Description

Allow whitelisted ribbon vaults to create a short position and direct their collateral to the new borrowable margin pool (MarginPoolV2) instead of the unborrowable marginpool (MarginPool). This allows certain whitelisted parties (whitelisted directly on MarginPoolV2) to borrow the collateral backing the call option. The whitelisted parties will exclusively be market makers who also buy the call option. This allows the market maker to delta hedge in a more capital efficient manner since they have an uncollateralized loan to the spot asset (which they can then short). We will whitelist vaults of treasuries that agree to offer this uncollateralized loan in exchange for higher premiums on the call option.

Say Zebra DAO has a $ZBR token. They launch a treasury vault with ribbon finance and deposit 100 $ZBR to sell covered calls and earn premiums from market makers. The 100 $ZBR is deposited into MarginPoolV2 when oTokens are minted. Market Maker X has 10 ZBR oTokens (call options) that they bought from the vault's oToken auction. Whereas in vanilla MarginPool the collateral cannot be taken out until the end, in MarginPoolV2 the oToken buyer (market maker) can borrow 10 $ZBR from MarginPoolV2 using borrow() to hedge their positive delta, posting ZERO collateral. Notice the amount they can borrow is proportional to how many oTokens they bought. Once a month passes, they deposit 10 $ZBR back into the vault.

Controller.sol:

setMarginPoolV2: adds margin pool v2 address
donateV2: donates to v2 margin pool

MarginPool.sol

setBorrowerWhitelistedStatus: sets whether an address is whitelisted to borrow
setRibbonVaultWhitelistedStatus: sets whether ribbon vault address is whitelisted to create short oToken on MarginPoolV2
setBuyerWhitelistedStatus: sets whether address is whitelisted as oToken buyer of oToken minted on MarginPoolV2
setBorrowPCT: sets the borrow percentage of entire collateral on a per-asset basis
borrow: allows whitelisted address (market maker) to borrow some of underlying collateral proportional to their oToken bal
borrowable: checks how much an address can borrow of underlying
repay: allows whitelisted address to repay their loan once oToken expires
repayFor: allows any address to repay loan on behalf of another party

Code

Unit test 100% coverage
Does your code follow the naming and code documentation guidelines?

Documentation

Is your code up to date with the spec?
Have you added your tests to the testing doc?

chudnov · 2022-07-19T11:37:10Z

contracts/core/MarginPoolV2.sol

+     */
+    function borrow(address _oToken, uint256 _amount) public onlyWhitelistedBorrower {
+        require(
+            WhitelistInterface(AddressBookInterface(addressBook).getWhitelist()).isWhitelistedOtoken(_oToken),


Check the Whitelist if the otoken is created. This means they have to create the otokens via the factory and prevents fake otokens to drain some underlying

chudnov · 2022-07-19T11:38:35Z

contracts/core/Controller.sol

+            ERC20Interface(collateral).balanceOf(address(poolV2)) < payout)
+            ? pool
+            : poolV2;
+


logic for _pool: in redeem() we have no reference to a vault where we can get the margin pool. This method just redeems collateral and the only given is the oToken address (no margin pool identification). So the way to guess the right margin pool to redeem from is to check if oToken owner is whitelisted in our ad-hoc mapping in pool v2. If it is whitelisted, we know for sure it is a market maker. But, it is still possible for market maker to hold oTokens for non-treasury vaults - in which case only first condition would be insufficient. We then check if the balance of that asset, like ETH, in the new margin pool is less than the payout. If it is then we know they are holding a normal otoken from say the ETH retail vault. Unless someone donates ETH for free to exclusively treasury vault margin pool, this will correctly identify which margin pool to retrieve from.

MM I'm not sure this is safe way to do it, in the example of MM holding oTokens for non-treasury vaults + treasury vaults, and they want to redeem the non-treasury one, and assuming the borrowable pool have enough balance, the amount will be deducted from the borrowable pool where it should be withdrawn from the normal pool, right?

Say MM1 have 500oToken, and 400oToken(belong to borrowable pool), and borrowable pool have 600USDC(for the 400oToken and other borrowers MM2,3...etc) MM1 come to redeem 500oToken, which should be redeemed from the normal pool, it will be redeemed from the borrowable pool

Also is there a scenario where it is more profitable for the borrower to not repay?

Responded redeem logic concerns at bottom.

Also is there a scenario where it is more profitable for the borrower to not repay?

It is always more profitable to not repay. That is why we make off-chain loan agreements with the market makers (taken care of by ribbon) - which is also a reason why we want to separate into another vault for this.

I haven't found an issue with this line yet, but it feels a bit uncomfortable for some reason to me.

If there are same assets in both regular and borrowable pools (say AAVE in borrowable and regular) then its possible a MM with oTokens that were bought from the regular retail vault and the treasury vault would claim all of their oTokens from the borrowable vault, when it really should be split between 2 pools. What will happen is later folks that have split oTokens fromm the retail and borrowable vaults will just redeem all of theirs against the regular pools, which seems ok, but feels a bit off.

chudnov · 2022-07-19T11:38:51Z

contracts/core/Controller.sol

@@ -719,6 +707,9 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
        // store new vault
        accountVaultCounter[_args.owner] = vaultId;
        vaultType[_args.owner][vaultId] = _args.vaultType;
+        vaults[_args.owner][vaultId].setMarginPool(
+            poolV2.whitelistedRibbonVault(_args.owner) ? address(poolV2) : address(pool)
+        );



when a vault is created, we immediately associate it with a margin pool in the Vault struct. We determine which margin pool to set by checking if owner of vault is a whitelisted address from the margin pool v2 ad-hoc mapping. Addresses of balancer, badger, spell vaults (and others) will be whitelisted. Retail vaults will not be and as a result will be directed to vanilla margin pool like before

chudnov · 2022-07-19T11:38:59Z

contracts/core/Controller.sol

@@ -458,26 +466,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
        return operators[_owner][_operator];
    }

-    /**


chudnov · 2022-07-19T11:39:03Z

contracts/core/Controller.sol

@@ -395,8 +396,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
     * @param _isOperator new boolean value that expresses if the sender is giving or revoking privileges for _operator
     */
    function setOperator(address _operator, bool _isOperator) external {
-        require(operators[msg.sender][_operator] != _isOperator, "C9");


Agee on removing here.

chudnov · 2022-07-19T11:39:06Z

contracts/core/Controller.sol

@@ -381,8 +384,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
     * @param _isRestricted new call restriction state
     */
    function setCallRestriction(bool _isRestricted) external onlyOwner {
-        require(callRestricted != _isRestricted, "C9");


Added comment about removing the require above

contracts/core/MarginPoolV2.sol

haythemsellami · 2022-07-21T20:18:48Z

contracts/core/Controller.sol

@@ -74,6 +72,7 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
    OracleInterface public oracle;
    MarginCalculatorInterface public calculator;
    MarginPoolInterface public pool;
+    MarginPoolInterface public poolV2;


Suggest renaming this to borrowablePool

haythemsellami · 2022-07-21T20:21:20Z

contracts/core/Controller.sol

    /**
     * @notice allows the partialPauser to toggle the systemPartiallyPaused variable and partially pause or partially unpause the system
     * @dev can only be called by the partialPauser
     * @param _partiallyPaused new boolean value to set systemPartiallyPaused to
     */
    function setSystemPartiallyPaused(bool _partiallyPaused) external onlyPartialPauser {
-        require(systemPartiallyPaused != _partiallyPaused, "C9");


IMO no harm for keeping those and it make it feel safer, also the owner is the only one capable of calling this function so it is fine to pay for extra gas

I would have left it, but otherwise I run into contract size issues and this seemed like the least consequential to remove.

haythemsellami · 2022-07-21T20:21:40Z

contracts/core/Controller.sol

@@ -343,8 +352,6 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
     * @param _fullyPaused new boolean value to set systemFullyPaused to
     */
    function setSystemFullyPaused(bool _fullyPaused) external onlyFullPauser {
-        require(systemFullyPaused != _fullyPaused, "C9");


Similar comment to the above require statement

haythemsellami · 2022-07-21T20:21:51Z

contracts/core/Controller.sol

-        require(_fullPauser != address(0), "C10");
-        require(fullPauser != _fullPauser, "C9");


Similar comment to the above require statement

Okay this was removed to avoid contract bytecode size, revisit later....

haythemsellami · 2022-07-21T20:22:02Z

contracts/core/Controller.sol

-        require(_partialPauser != address(0), "C11");
-        require(partialPauser != _partialPauser, "C9");


Similar comment to the above require statement

Okay this was removed to avoid contract bytecode size, revisit later...

haythemsellami · 2022-07-21T20:30:41Z

contracts/core/Controller.sol

+    /**
+     * @notice Sets margin pool v2
+     * @dev can only be called by the owner
+     * @param _marginPoolV2 margin pool v2 address
+     */
+    function setMarginPoolV2(address _marginPoolV2) external onlyOwner {
+        poolV2 = MarginPoolInterface(_marginPoolV2);
+    }
+


This one can be removed, the borrowablePool address should be set in the AddressBook (following the current architecture) using the AddressBook.setAddress() function, then the address can be fetched and assigned to this variable in the _refreshConfigInternal()

borrowablePool = MarginPoolInterface(addressbook. getAddress(keccak256"BORROWABLE_POOL"));

Ah, I didn't realize this was possible. Good solution!

haythemsellami · 2022-07-21T20:31:15Z

contracts/core/Controller.sol

-     * @return calculator, the address of the calculator module
-     * @return pool, the address of the pool module
-     */
-    function getConfiguration()


Why removing this one?

contract size

haythemsellami · 2022-07-21T20:38:00Z

contracts/libs/MarginVault.sol

@@ -31,6 +31,8 @@ library MarginVault {

    // vault is a struct of 6 arrays that describe a position a user has, a user can have multiple vaults.
    struct Vault {
+        // address of the margin pool for vault


IMO it is better to keep using vaultType instead of adding address pool in the vault structure, currently we have 2 vault types, we can use 2 as borrowable vault, and then do the checks if vault type is 3 when depositing collateral, forward it to the borrowablePool

changing the vault struct will mess up with all existing vault in the storage right? or is it safe because all existing properties are arrays so they're not stored based on order?

Yes I will set vault type index 2 to borrowable pool instead of setting pool in vault struct. Upon second glance I realize this won't cause any problems with payoff calculation / liquidation logic in MarginCalculator because this pool will only be used for covered calls (not a naked vault) so the pool and naked vaults are mutually exclusive.

haythemsellami

IMO better to remove the margin pool address from the vault data struct and use a new vault type 3 for borrowable vault, and if a vault is type 3, then forward the collateral to deposit to borrowablePool (basically vault type 3 interact with the new pool) + checking if the vault is whitelisted to be borrowable one (basically similar to the checks implemented now)

haythemsellami · 2022-07-21T20:42:05Z

contracts/core/Controller.sol

@@ -719,6 +707,9 @@ contract Controller is Initializable, OwnableUpgradeSafe, ReentrancyGuardUpgrade
        // store new vault
        accountVaultCounter[_args.owner] = vaultId;
        vaultType[_args.owner][vaultId] = _args.vaultType;
+        vaults[_args.owner][vaultId].setMarginPool(
+            poolV2.whitelistedRibbonVault(_args.owner) ? address(poolV2) : address(pool)


I guess we can rename this check function to isWhitelistedBorrowableVault()

haythemsellami · 2022-07-21T20:43:00Z

contracts/core/MarginPoolV2.sol

+    /// collateral in this pool
+    mapping(address => bool) public whitelistedOTokenBuyer;
+    /// @dev mapping between address and whitelist status of ribbon vault
+    mapping(address => bool) public whitelistedRibbonVault;


Rename to a generic naming whitelistedBorrowableVault

antoncoding · 2022-07-22T13:30:06Z

contracts/core/MarginPoolV2.sol

+
+        require(!oToken.isPut(), "MarginPool: oToken is not a call option");
+
+        require(borrowPCT[collateralAsset] > 0, "MarginPool: Borrowing is paused for collateral asset");


i think this is a duplicate check with L155

require(_amount <= collateralAllocatedToMM.mul(borrowPCT[collateralAsset]).div(TOTAL_PCT).sub(outstandingAssetBorrow), "MarginPool: Borrowing more than allocated" );

Can remove to save gas

antoncoding · 2022-07-22T13:36:48Z

contracts/core/MarginPoolV2.sol

+        // transfer _asset _amount from pool to borrower
+        ERC20Interface(collateralAsset).safeTransfer(msg.sender, _amount);
+        emit Borrow(_oToken, collateralAsset, _amount, msg.sender);


Two tricks that would probably mess up the accounting:

whitelisted parties can collude and make others unable to borrow: they can transfer the oToken balance back and forth among whitelisted parties and borrow more than what they should have.

whitelisted parties can flash mint bunch of oTokens and borrow almost every borrowable asset from the pool.

I think the pool should lock up oToken when someone come and borrow, to prevent these scenarios

Scenario 1 is possible. I just chalked it up to "master loan agreement will be signed so there is no incentive here" but locking up oTokens definitely solves this.

Scenario 2 flashminting is solved because whereas before they could flash mint whitelisted oToken and borrow underlying from pool and then just burn the original oTokens - with a lock they are basically swapping their own collateral with that of the pools so there is no gain here.

@antoncoding @haythem96 thanks for the thorough review

haythemsellami · 2022-07-23T11:17:30Z

contracts/core/MarginPoolV2.sol

+ * @title MarginPoolV2
+ * @notice Contract that holds all protocol funds AND allows collateral borrows
+ */
+contract MarginPoolV2 is MarginPool {


BorrowableMarginPool make more sense as naming than MarginPoolV2

haythemsellami

Overall LGTM, left few more comments. Would love also @aleone review

haythemsellami · 2022-07-27T14:03:55Z

@akwritescode Can you please check circleci, because some jobs can't be executed from outside opyn org members.

haythemsellami · 2022-07-27T14:05:09Z

When CI successfully run, based on coverage may need to add some tests, especially one to cover cases where Controller is involved

chudnov · 2022-07-27T22:44:03Z

When CI successfully run, based on coverage may need to add some tests, especially one to cover cases where Controller is involved

can you elaborate?

chudnov · 2022-07-27T22:44:47Z

@chuddster Most of it looks fine to me.

Based on the current implementation, agree on:

Keeping the new whitelist functions in the new BorrowablePool, no need to upgrade Whitelist module...etc

Current naming LGTM

I still think the vault type set when opening a vault should change, the proposed implementation in the comment is more generic and does not assume that user who will use the borrowable pool will not need to open spread/naked margin #459 (comment)

Yes in general I agree your approach is better design (letting owner pass it through) instead of overriding whatever value is passed. Currently we pass in vault type 0 in these treasury vaults which will be overridden with this upgraded controller. This implementation does not require any vaults to be redeployed - we can just call setWhitelistedOptionsVault(true) on all existing addresses and it will override the vault type to 2. We already have 4 treasury vaults that are deployed and have funds in them and we would have to redeploy all of them and pass in vault type 2. This would require all of the treasuries to migrate to a new contract which is undesirable. I agree its a bit hacky.

The only vaults that will be able to open a margin vault in the borrowable pool are ribbon vaults (we will whitelist them) - so we are only constraining ourselves if we ever want to do a naked margin vault.

haythemsellami · 2022-07-28T09:03:08Z

When CI successfully run, based on coverage may need to add some tests, especially one to cover cases where Controller is involved

can you elaborate?

Just meant when CI run, to check the coverage if there are some important missing paths not tested.

aleone

Generally looks good to me with a few comments throughout that would like a look at.

Main comment is on the accounting and slight feeling of unease around how exercise occurs on the different vaults (but can't figure out an issue with it yet). There are some tests where new borrowable margin pool is tested but testing of non-borrowable is removed, I'd like the test to be additive and not remove existing testing. If we make changes to something in future, want to have both of them fully tested to prevent missing coverage/test cases etc.

I also think it makes sense to at least add 1 integration test flow w non-mocked contracts vs all in a unit test w mocks.

aleone · 2022-08-06T00:12:00Z

contracts/core/BorrowableMarginPool.sol

+     * @param _oToken address of the oToken laying claim to the collateral assets
+     * @param _oTokenAmount amount of the oToken to post as collateral in exchange
+     */
+    function borrow(address _oToken, uint256 _oTokenAmount) public onlyWhitelistedBorrower {


I'm a bit confused by why the accounting was done as it was. Let me know if I'm missing something.

The way its done is - figure out how much asset can be borrowed by balanceOf(msg.sender) and borrowPCT, then give collateral out 1:1 based on oTokens provided, respecting the max that can be borrowed based on current balanceOf(msg.sender). However, someone can just send the oTokens that they have held to some other whitelisted address after borrowing, allowing the other party to borrow more. A MM can also sell their tokens allowing them to borrow more than they should be permitted to do, as they didn't have to put in collateralBorrowed/borrowPCT oTokens.

Is the depositing of oTokens just meant as a sort of receipt (and don't really care if they violate the borrowPCT later)? It seems like it works ok for that, but it seems pretty easy to get around the borrow limits / exceed them by changing balances after borrowing.

Why don't you only let the borrower borrow the amount of oToken provided * the borrow percent? Ie right now if borrow % is 50%, you provide 100 oTokens to borrow 100 underlyingAssets, but it requires that your balanceOf >= 200 oTokens before the transfer. Why not require them to put up 200 oTokens to borrow 100 underlyingAssets?

However, someone can just send the oTokens that they have held to some other whitelisted address after borrowing, allowing the other party to borrow more.

Yes, the following is possible:

Whitelisted Borrower A has 100 oTokens, borrowPCT is 50% Whitelisted Borrower B has 100 oTokens, borrowPCT is 100% Borrower A borrowers 50 of underlying, contract locks 50 oTokens Borrower A transfers rest of 50 oTokens to Borrower B Borrower B has 150 oTokens, borrowPCT is 100% Borrower B borrows 50 of underlying, contract locks 50 oTokens Borrower B transfers 50 of underlying back to Borrower A

End result: Borrower A was able to circumvent the borrow limit by colluding with Borrower B who has not sacrificed anything in the process. We do rely on KYC/AML here and the reputation of these market makers to not do this. After all Borrower B is risking his reputation / not being able to borrow in the future by doing this. This loophole is easy to monitor and I don't see a way of getting around this on a smart contract level.

A MM can also sell their tokens allowing them to borrow more than they should be permitted to do, as they didn't have to put in collateralBorrowed/borrowPCT oTokens

What exactly do you mean? Selling oTokens prevents them from borrowing underlying.

Is the depositing of oTokens just meant as a sort of receipt

Locking solves the two issues mentioned in anton's comment. This does not solve the issue above but at least they cannot borrow more of the underlying than the oTokens they actually bought (essentially stealing from other whitelisted borrowers).

Why not require them to put up 200 oTokens to borrow 100 underlyingAssets?

We could, but I don't see how it prevents getting around borrow limits in situation described above. Just can transfer the oTokens beforehand to some whitelisted market maker with a higher borrow limit.

Referring to comment below

Reasoning is here. TL;DR we make sure that there is no asset intersection between borrowable and non-borrowable margin pools for whitelisted market makers oTokens. As you said it is a bit odd if this does end up happening but redeem transactions will still succeed.

I'm fine with this, just wanted to highlight the potential issues/pitfalls.

I think the way you prevent it, if you wanted, would just be to make the person lock up the assetRequested / borrowPCT in options. So if borrowPCT was 50% and someone wanted 50 of the asset, they would have to lock up 100 options. That would prevent them from 1) selling in future 2) transferring the extra 50 to someone else later.

What exactly do you mean? Selling oTokens prevents them from borrowing underlying.

I meant that if borrowPCT is 50%, and a MM has 100 tokens, they only have to lock 50 of them up to borrow 50 assets. They can then sell 50 oTokens and circumvent the borrow limit after borrowing the asset.

aleone · 2022-08-06T00:26:36Z

contracts/core/BorrowableMarginPool.sol

+        // transfer _oTokenAmount of oToken from borrower to _pool
+        ERC20Interface(_oToken).safeTransferFrom(msg.sender, address(this), _oTokenAmount);
+        // transfer collateralAssetAmount of collateralAsset from pool to borrower
+        ERC20Interface(collateralAsset).safeTransfer(msg.sender, collateralAssetAmount);
+        emit Borrow(_oToken, collateralAsset, collateralAssetAmount, msg.sender);


I don't really see an issue if don't update the oToken balances, but it does seem more consistent to keep them updated along with the collateral, but ok with either way.

aleone · 2022-08-06T00:49:31Z

contracts/core/Controller.sol

+            ERC20Interface(collateral).balanceOf(address(poolV2)) < payout)
+            ? pool
+            : poolV2;
+


I haven't found an issue with this line yet, but it feels a bit uncomfortable for some reason to me.

If there are same assets in both regular and borrowable pools (say AAVE in borrowable and regular) then its possible a MM with oTokens that were bought from the regular retail vault and the treasury vault would claim all of their oTokens from the borrowable vault, when it really should be split between 2 pools. What will happen is later folks that have split oTokens fromm the retail and borrowable vaults will just redeem all of theirs against the regular pools, which seems ok, but feels a bit off.

aleone · 2022-08-06T01:16:51Z

test/unit-tests/controller.test.ts

@@ -314,10 +320,17 @@ contract(

        const vaultCounterAfter = new BigNumber(await controllerProxy.getAccountVaultCounter(accountOwner1))
        assert.equal(vaultCounterAfter.minus(vaultCounterBefore).toString(), '1', 'vault counter after mismatch')
+        assert.equal(finalMarginPool, marginPool.address, 'vault margin pool mismatch')
      })

      it('should open vault from account operator', async () => {


prb should be more explicit, this testing operator and borrowableVault now

removing as it does not belong in this test

aleone · 2022-08-06T02:17:18Z

test/unit-tests/borrowableMarginPool.test.ts

+      )
+    })
+
+    it('should revert borrowing after borrowing full amount', async () => {


I think this is testing same as above, not after any borrow of full amount - its just a user with no balance.

Yes it is. Will delete.

test/unit-tests/marginCalculator.test.ts

test/unit-tests/borrowableMarginPool.test.ts

aleone · 2022-08-06T02:45:54Z

test/unit-tests/controller.test.ts

      })

      it('should open vault from account operator', async () => {
+        await borrowableMarginPool.setOptionsVaultWhitelistedStatus(accountOwner1, true, { from: owner })


I don't like how in this test pretty much all of the accountOwner1 tests are now only done on the borrowable margin vault. I'd prefer that we do on two different owners one that is borrowable and one that is regular and do tests on each. Could even just create a 2nd unit test which repeats all the code but just whitelists the one account in one case and doesn't in 2nd.

aleone · 2022-08-06T02:48:23Z

test/unit-tests/controller.test.ts

@@ -2707,11 +2839,19 @@ contract(
            data: ZERO_ADDR,
          },
        ]
-        await usdc.approve(marginPool.address, collateralToDeposit, { from: accountOwner1 })
+        await borrowableMarginPool.setOTokenBuyerWhitelistedStatus(holder1, true, { from: owner })
+        finalMarginPool =


We should test this flow, but it should be in its own file vs removing test functionality that exists (add vs change tests).

I added a controllerBorrowableMarginPoo.test.ts in addition to thecontroller.test.ts.

In controllerBorrowableMarginPoo.test.ts I test the changed flow.
In controller.test.ts I test the flow from this comment as well as from comment above (non borrowable accountOwner1)

…that is whitelisted, remove unnecessary dictionary key-value in marginCalculator

aleone

lgtm

aleone · 2022-08-23T03:55:09Z

contracts/core/BorrowableMarginPool.sol

+     * @param _oToken address of the oToken laying claim to the collateral assets
+     * @param _oTokenAmount amount of the oToken to post as collateral in exchange
+     */
+    function borrow(address _oToken, uint256 _oTokenAmount) public onlyWhitelistedBorrower {


I'm fine with this, just wanted to highlight the potential issues/pitfalls.

I think the way you prevent it, if you wanted, would just be to make the person lock up the assetRequested / borrowPCT in options. So if borrowPCT was 50% and someone wanted 50 of the asset, they would have to lock up 100 options. That would prevent them from 1) selling in future 2) transferring the extra 50 to someone else later.

What exactly do you mean? Selling oTokens prevents them from borrowing underlying.

I meant that if borrowPCT is 50%, and a MM has 100 tokens, they only have to lock 50 of them up to borrow 50 assets. They can then sell 50 oTokens and circumvent the borrow limit after borrowing the asset.

This reverts commit 841f81d.

Your Name added 2 commits July 19, 2022 19:13

Add smart contract changes

cb5ccff

Add tests

2d2793b

chudnov force-pushed the borrow-margin-pool branch from 76e116c to 2d2793b Compare July 19, 2022 11:25

Merge branch 'master' into borrow-margin-pool

ca1fd8d

chudnov commented Jul 19, 2022

View reviewed changes

contracts/core/MarginPoolV2.sol Outdated Show resolved Hide resolved

haythemsellami requested review from haythemsellami, aparnakr, aleone and DemolaJames July 21, 2022 20:15

haythemsellami reviewed Jul 21, 2022

View reviewed changes

haythemsellami requested changes Jul 21, 2022

View reviewed changes

haythemsellami reviewed Jul 21, 2022

View reviewed changes

antoncoding reviewed Jul 22, 2022

View reviewed changes

haythemsellami reviewed Jul 23, 2022

View reviewed changes

move borrowable pool address into address book

eeac3b8

haythemsellami approved these changes Jul 27, 2022

View reviewed changes

subtraction overflow checks and change asset balance

ee44e5e

chudnov requested a review from haythemsellami July 27, 2022 23:02

Your Name added 2 commits July 27, 2022 18:56

rerun unit test

9c4eefc

remove changes to liquidations.ts

56b51be

haythemsellami previously approved these changes Jul 28, 2022

View reviewed changes

prevent whitelist of retail options vaults

3850647

DemolaJames previously approved these changes Aug 3, 2022

View reviewed changes

aleone reviewed Aug 6, 2022

View reviewed changes

Add new controllerBorrowableMarginPool.test.ts testing accountOwner1 …

20b6841

…that is whitelisted, remove unnecessary dictionary key-value in marginCalculator

chudnov dismissed stale reviews from DemolaJames and haythemsellami via 20b6841 August 10, 2022 02:37

fix unit test

660d046

haythemsellami requested review from DemolaJames, haythemsellami and aleone August 10, 2022 16:13

haythemsellami previously approved these changes Aug 10, 2022

View reviewed changes

test integration flow with borrowable pool

431764b

chudnov dismissed haythemsellami’s stale review via 431764b August 15, 2022 17:07

haythemsellami self-requested a review August 16, 2022 13:39

haythemsellami approved these changes Aug 19, 2022

View reviewed changes

aleone approved these changes Aug 23, 2022

View reviewed changes

aleone merged commit 841f81d into opynfinance:master Aug 23, 2022

haythemsellami pushed a commit that referenced this pull request Dec 15, 2022

Revert "Add Borrowable Margin Pool (V2) (#459)"

7f1375f

This reverts commit 841f81d.

haythemsellami mentioned this pull request Dec 15, 2022

Revert "Add Borrowable Margin Pool (V2)" #467

Open

		require(_fullPauser != address(0), "C10");
		require(fullPauser != _fullPauser, "C9");

		require(_partialPauser != address(0), "C11");
		require(partialPauser != _partialPauser, "C9");


		require(!oToken.isPut(), "MarginPool: oToken is not a call option");

		require(borrowPCT[collateralAsset] > 0, "MarginPool: Borrowing is paused for collateral asset");

Add Borrowable Margin Pool (V2) #459

Add Borrowable Margin Pool (V2) #459

Conversation

chudnov commented Jul 19, 2022 • edited Loading

Task: Feature Name:

High Level Description

Code

Documentation

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haythemsellami Jul 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chudnov Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

haythemsellami left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chudnov Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haythemsellami Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

haythemsellami left a comment

Choose a reason for hiding this comment

haythemsellami commented Jul 27, 2022

haythemsellami commented Jul 27, 2022

chudnov commented Jul 27, 2022

chudnov commented Jul 27, 2022 • edited Loading

haythemsellami commented Jul 28, 2022

aleone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chudnov Aug 10, 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

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

aleone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chudnov commented Jul 19, 2022 •

edited

Loading

haythemsellami Jul 21, 2022 •

edited

Loading

chudnov Jul 25, 2022 •

edited

Loading

haythemsellami left a comment •

edited

Loading

chudnov Jul 25, 2022 •

edited

Loading

haythemsellami Jul 23, 2022 •

edited

Loading

chudnov commented Jul 27, 2022 •

edited

Loading

chudnov Aug 10, 2022 •

edited

Loading