Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: reduce base strategy size #422

Merged
2 commits merged into from Jun 29, 2021
Merged

refactor: reduce base strategy size #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
55866bf
refactor: reduce base strategy size
Jun 25, 2021
0193c61
fix: do not add onlyManagement
Jun 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 34 additions & 31 deletions contracts/BaseStrategy.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,39 +272,30 @@ abstract contract BaseStrategy {
bool public emergencyExit;

// modifiers
modifier onlyAuthorized() {
require(msg.sender == strategist || msg.sender == governance(), "!authorized");
_;
function _onlyAuthorized() internal {
require(msg.sender == strategist || msg.sender == governance());
}

modifier onlyEmergencyAuthorized() {
require(
msg.sender == strategist || msg.sender == governance() || msg.sender == vault.guardian() || msg.sender == vault.management(),
"!authorized"
);
_;
function _onlyEmergencyAuthorized() internal {
require(msg.sender == strategist || msg.sender == governance() || msg.sender == vault.guardian() || msg.sender == vault.management());
}

modifier onlyStrategist() {
require(msg.sender == strategist, "!strategist");
_;
function _onlyStrategist() internal {
require(msg.sender == strategist);
}

modifier onlyGovernance() {
require(msg.sender == governance(), "!authorized");
_;
function _onlyGovernance() internal {
require(msg.sender == governance());
}

modifier onlyKeepers() {
function _onlyKeepers() internal {
require(
msg.sender == keeper ||
msg.sender == strategist ||
msg.sender == governance() ||
msg.sender == vault.guardian() ||
msg.sender == vault.management(),
"!authorized"
msg.sender == vault.management()
);
_;
}

constructor(address _vault) public {
Expand Down Expand Up @@ -354,7 +345,8 @@ abstract contract BaseStrategy {
* This may only be called by governance or the existing strategist.
* @param _strategist The new address to assign as `strategist`.
*/
function setStrategist(address _strategist) external onlyAuthorized {
function setStrategist(address _strategist) external {
_onlyAuthorized();
require(_strategist != address(0));
strategist = _strategist;
emit UpdatedStrategist(_strategist);
Expand All @@ -373,7 +365,8 @@ abstract contract BaseStrategy {
* This may only be called by governance or the strategist.
* @param _keeper The new address to assign as `keeper`.
*/
function setKeeper(address _keeper) external onlyAuthorized {
function setKeeper(address _keeper) external {
_onlyAuthorized();
require(_keeper != address(0));
keeper = _keeper;
emit UpdatedKeeper(_keeper);
Expand All @@ -387,7 +380,8 @@ abstract contract BaseStrategy {
* This may only be called by the strategist.
* @param _rewards The address to use for pulling rewards.
*/
function setRewards(address _rewards) external onlyStrategist {
function setRewards(address _rewards) external {
_onlyStrategist();
require(_rewards != address(0));
vault.approve(rewards, 0);
rewards = _rewards;
Expand All @@ -407,7 +401,8 @@ abstract contract BaseStrategy {
* This may only be called by governance or the strategist.
* @param _delay The minimum number of seconds to wait between harvests.
*/
function setMinReportDelay(uint256 _delay) external onlyAuthorized {
function setMinReportDelay(uint256 _delay) external {
_onlyAuthorized();
minReportDelay = _delay;
emit UpdatedMinReportDelay(_delay);
}
Expand All @@ -424,7 +419,8 @@ abstract contract BaseStrategy {
* This may only be called by governance or the strategist.
* @param _delay The maximum number of seconds to wait between harvests.
*/
function setMaxReportDelay(uint256 _delay) external onlyAuthorized {
function setMaxReportDelay(uint256 _delay) external {
_onlyAuthorized();
maxReportDelay = _delay;
emit UpdatedMaxReportDelay(_delay);
}
Expand All @@ -439,7 +435,8 @@ abstract contract BaseStrategy {
* @param _profitFactor A ratio to multiply anticipated
* `harvest()` gas cost against.
*/
function setProfitFactor(uint256 _profitFactor) external onlyAuthorized {
function setProfitFactor(uint256 _profitFactor) external {
_onlyAuthorized();
profitFactor = _profitFactor;
emit UpdatedProfitFactor(_profitFactor);
}
Expand All @@ -457,7 +454,8 @@ abstract contract BaseStrategy {
* @param _debtThreshold How big of a loss this Strategy may carry without
* being required to report to the Vault.
*/
function setDebtThreshold(uint256 _debtThreshold) external onlyAuthorized {
function setDebtThreshold(uint256 _debtThreshold) external {
_onlyAuthorized();
debtThreshold = _debtThreshold;
emit UpdatedDebtThreshold(_debtThreshold);
}
Expand All @@ -470,7 +468,8 @@ abstract contract BaseStrategy {
* This may only be called by governance or the strategist.
* @param _metadataURI The URI that describe the strategy.
*/
function setMetadataURI(string calldata _metadataURI) external onlyAuthorized {
function setMetadataURI(string calldata _metadataURI) external {
_onlyAuthorized();
metadataURI = _metadataURI;
emit UpdatedMetadataURI(_metadataURI);
}
Expand Down Expand Up @@ -638,7 +637,8 @@ abstract contract BaseStrategy {
*
* This may only be called by governance, the strategist, or the keeper.
*/
function tend() external onlyKeepers {
function tend() external {
_onlyKeepers();
// Don't take profits with this call, but adjust for better gains
adjustPosition(vault.debtOutstanding());
}
Expand Down Expand Up @@ -725,7 +725,8 @@ abstract contract BaseStrategy {
* called to report to the Vault on the Strategy's position, especially if
* any losses have occurred.
*/
function harvest() external onlyKeepers {
function harvest() external {
_onlyKeepers();
uint256 profit = 0;
uint256 loss = 0;
uint256 debtOutstanding = vault.debtOutstanding();
Expand Down Expand Up @@ -809,7 +810,8 @@ abstract contract BaseStrategy {
* @dev
* See `vault.setEmergencyShutdown()` and `harvest()` for further details.
*/
function setEmergencyExit() external onlyEmergencyAuthorized {
function setEmergencyExit() external {
_onlyEmergencyAuthorized();
emergencyExit = true;
vault.revokeStrategy();

Expand Down Expand Up @@ -853,7 +855,8 @@ abstract contract BaseStrategy {
* should be protected from sweeping in addition to `want`.
* @param _token The token to transfer out of this vault.
*/
function sweep(address _token) external onlyGovernance {
function sweep(address _token) external {
_onlyGovernance();
require(_token != address(want), "!want");
require(_token != address(vault), "!shares");

Expand Down
23 changes: 11 additions & 12 deletions tests/functional/strategy/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,14 @@ def test_strategy_harvest_permission(


@pytest.mark.parametrize(
"getter,setter,val,gov_allowed,strategist_allowed,authority_error",
"getter,setter,val,gov_allowed,strategist_allowed",
[
("rewards", "setRewards", None, False, True, "!strategist"),
("keeper", "setKeeper", None, True, True, "!authorized"),
("minReportDelay", "setMinReportDelay", 1000, True, True, "!authorized"),
("maxReportDelay", "setMaxReportDelay", 2000, True, True, "!authorized"),
("profitFactor", "setProfitFactor", 1000, True, True, "!authorized"),
("debtThreshold", "setDebtThreshold", 1000, True, True, "!authorized"),
("rewards", "setRewards", None, False, True),
("keeper", "setKeeper", None, True, True),
("minReportDelay", "setMinReportDelay", 1000, True, True),
("maxReportDelay", "setMaxReportDelay", 2000, True, True),
("profitFactor", "setProfitFactor", 1000, True, True),
("debtThreshold", "setDebtThreshold", 1000, True, True),
],
)
def test_strategy_setParams(
Expand All @@ -94,7 +94,6 @@ def test_strategy_setParams(
val,
gov_allowed,
strategist_allowed,
authority_error,
):
if val is None:
# Can't access fixtures, so use None to mean any random address
Expand All @@ -103,7 +102,7 @@ def test_strategy_setParams(
prev_val = getattr(strategy, getter)()

# None of these params can be set by a rando
with brownie.reverts(authority_error):
with brownie.reverts():
getattr(strategy, setter)(val, {"from": rando})

def try_setParam(caller, allowed):
Expand All @@ -114,7 +113,7 @@ def try_setParam(caller, allowed):
getattr(strategy, setter)(prev_val, {"from": caller})
assert getattr(strategy, getter)() == prev_val
else:
with brownie.reverts(authority_error):
with brownie.reverts():
getattr(strategy, setter)(val, {"from": caller})

try_setParam(strategist, strategist_allowed)
Expand All @@ -126,14 +125,14 @@ def test_set_strategist_authority(strategy, strategist, rando):
# so this test handles it.

# Only gov or strategist can setStrategist
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.setStrategist(rando, {"from": rando})

# As strategist, set strategist to rando.
strategy.setStrategist(rando, {"from": strategist})

# Now the original strategist shouldn't be able to set strategist again
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.setStrategist(rando, {"from": strategist})


Expand Down
8 changes: 4 additions & 4 deletions tests/functional/strategy/test_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ def test_harvest_tend_authority(gov, keeper, strategist, strategy, rando, chain)
strategy.tend({"from": keeper})
strategy.tend({"from": strategist})
strategy.tend({"from": gov})
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.tend({"from": rando})

# Only keeper, strategist, or gov can call harvest
Expand All @@ -20,7 +20,7 @@ def test_harvest_tend_authority(gov, keeper, strategist, strategy, rando, chain)
strategy.harvest({"from": strategist})
chain.sleep(1)
strategy.harvest({"from": gov})
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.harvest({"from": rando})


Expand Down Expand Up @@ -146,7 +146,7 @@ def test_sweep(gov, vault, strategy, rando, token, other_token):
assert other_token.balanceOf(strategy) > 0
assert other_token.balanceOf(gov) == 0
# Not any random person can do this
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.sweep(other_token, {"from": rando})

before = other_token.balanceOf(strategy)
Expand Down Expand Up @@ -184,7 +184,7 @@ def test_set_metadataURI(gov, strategy, strategist, rando):
assert strategy.metadataURI() == "ipfs://test2"
strategy.setMetadataURI("ipfs://test3", {"from": strategist})
assert strategy.metadataURI() == "ipfs://test3"
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.setMetadataURI("ipfs://fake", {"from": rando})


Expand Down
4 changes: 2 additions & 2 deletions tests/functional/strategy/test_shutdown.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,9 +136,9 @@ def test_set_emergency_exit_authority(
strategy, gov, strategist, keeper, rando, management, guardian
):
# Can only setEmergencyExit as governance, strategist, vault management and guardian
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.setEmergencyExit({"from": keeper})
with brownie.reverts("!authorized"):
with brownie.reverts():
strategy.setEmergencyExit({"from": rando})
strategy.setEmergencyExit({"from": gov})
brownie.chain.undo()
Expand Down