C4 mothership fixes #119
C4 mothership fixes #119
Conversation
…core into c4-mothership
| uint16 public constant MIN_FEE = 1; // 0.01% | ||
| uint16 public constant MAX_FEE = 100; // 1.00% | ||
| uint public constant MIN_FEE = 1e14; // 0.01% | ||
| uint public constant MAX_FEE = 1e16; // 1.00% |
There was a problem hiding this comment.
Can we see scenarios on long tail markets where we want more?
| @@ -60,65 +60,59 @@ contract OverlayV1Mothership is AccessControlEnumerable { | |||
|
|
|||
| _setupRole(ADMIN, msg.sender); | |||
| _setupRole(GOVERNOR, msg.sender); | |||
| _setupRole(GUARDIAN, msg.sender); | |||
There was a problem hiding this comment.
Why are we removing the guardian role? I don't have any strong opinions yet about the types of roles we need but it is likely more than just admin and governor.
Governor would be something with a reasonable time lock for instance.
Guardian would be something along the lines of emergency shutdown.
Admin, that'd be to grant/remove various roles.
Not sure though. I'd like to look at other protocol's thought processes. Fei in particular. We are in the realm of somewhat more permissioned operation so maybe protocols like Aave and Compound should also be taken into account.
|
|
||
| marketExists[market] = true; | ||
| marketActive[market] = true; | ||
| function initializeMarket(address _market) external onlyGovernor { |
There was a problem hiding this comment.
This isn't really initializing a market, that seems to say we are instantiating some state on the market. We are just adding it to the manifest here in the mothership.
| @@ -128,65 +122,92 @@ contract OverlayV1Mothership is AccessControlEnumerable { | |||
| @param _collateral Overlay OVL collateral contract address | |||
| */ | |||
| function initializeCollateral (address _collateral) external onlyGovernor { | |||
There was a problem hiding this comment.
same, not really initializing here, just adding to the manifest and activating
| market_path + | ||
| '_raw_uni_framed.json') | ||
| market_path | ||
| + '_raw_uni_framed.json') |
There was a problem hiding this comment.
Why? Changes like this exaggerate pull requests and might lead to annoying conflicts? Just leave it the way it was in the future
|
|
||
|
|
||
| def test_initialize_collateral(mothership, collateral, gov, token): | ||
| total = mothership.totalCollaterals() |
There was a problem hiding this comment.
nitpick, I would name this initial_total
| mothership.disableCollateral(collateral, {"from": gov}) | ||
|
|
||
|
|
||
| def test_disable_collateral_reverts_when_not_gov(mothership, collateral, gov, |
There was a problem hiding this comment.
Here is a function we might also want to have a guardian role authorized for. If something is going haywire, we need immediate action.
| @@ -0,0 +1,160 @@ | |||
| import brownie | |||
There was a problem hiding this comment.
I would name this file test_collateral_administration.py
| @@ -0,0 +1,136 @@ | |||
| import brownie | |||
There was a problem hiding this comment.
I would name this file test_market_administration.py
Mitigates issues:
OverlayV1Mothership#marginBurnRatecode-423n4/2021-11-overlay-findings#84