feat: review improvements (#21)

* feat: minor improvements from review

* test: correct broken tests

* test: more coverage

* chore: add reminder to improve this fixture

contracts/RewardsHandler.vy

@@ -83,6 +83,7 @@ exports: (
 event MinimumWeightUpdated:
     new_minimum_weight: uint256
 
+
 event ScalingFactorUpdated:
     new_scaling_factor: uint256
 
@@ -93,6 +94,7 @@ event ScalingFactorUpdated:
 
 
 RATE_MANAGER: public(constant(bytes32)) = keccak256("RATE_MANAGER")
+RECOVERY_MANAGER: public(constant(bytes32)) = keccak256("RECOVERY_MANAGER")
 WEEK: constant(uint256) = 86400 * 7  # 7 days
 MAX_BPS: constant(uint256) = 10**4  # 100%
 
@@ -142,8 +144,9 @@ def __init__(
     access_control.__init__()
     # admin (most likely the dao) controls who can be a rate manager
     access_control._grant_role(access_control.DEFAULT_ADMIN_ROLE, admin)
-    # admin itself is a RATE_ADMIN
+    # admin itself is a RATE_MANAGER and RECOVERY_MANAGER
     access_control._grant_role(RATE_MANAGER, admin)
+    access_control._grant_role(RECOVERY_MANAGER, admin)
     # deployer does not control this contract
     access_control._revoke_role(access_control.DEFAULT_ADMIN_ROLE, msg.sender)
 
@@ -204,12 +207,13 @@ def process_rewards():
         self.distribution_time != 0
     ), "rewards should be distributed over time"
 
-
     # any crvUSD sent to this contract (usually through the fee splitter, but
     # could also come from other sources) will be used as a reward for crvUSD
     # stakers in the vault.
     available_balance: uint256 = staticcall stablecoin.balanceOf(self)
 
+    assert available_balance > 0, "no rewards to distribute"
+
     # we distribute funds in 2 steps:
     # 1. transfer the actual funds
     extcall stablecoin.transfer(vault.address, available_balance)
@@ -247,7 +251,9 @@ def weight() -> uint256:
     for more at the beginning and can also be increased in the future if someone
     tries to manipulate the time-weighted average of the tvl ratio.
     """
-    return max(twa._compute() * self.scaling_factor // MAX_BPS, self.minimum_weight)
+    return max(
+        twa._compute() * self.scaling_factor // MAX_BPS, self.minimum_weight
+    )
 
 
 ################################################################
@@ -346,7 +352,7 @@ def recover_erc20(token: IERC20, receiver: address):
     to this contract. crvUSD cannot be recovered as it's part of the core logic of
     this contract.
     """
-    access_control._check_role(RATE_MANAGER, msg.sender)
+    access_control._check_role(RECOVERY_MANAGER, msg.sender)
 
     # if crvUSD was sent by accident to the contract the funds are lost and will
     # be distributed as staking rewards on the next `process_rewards` call.

tests/unitary/conftest.py

@@ -11,6 +11,7 @@ def yearn_gov():
 
 @pytest.fixture(scope="module")
 def curve_dao():
+    # TODO add a fixture for rate managers that contains curve dao
     return boa.env.generate_address()
 
 

tests/unitary/rewards_handler/test_process_rewards.py

@@ -0,0 +1,29 @@
+import boa
+
+
+def test_default_behavior(rewards_handler, crvusd, vault, curve_dao):
+    distributed_amount = 10**18 * 100
+    boa.deal(crvusd, rewards_handler, distributed_amount)
+
+    rewards_handler.set_distribution_time(86400 * 7, sender=curve_dao)
+
+    assert crvusd.balanceOf(rewards_handler) == distributed_amount
+
+    rewards_handler.process_rewards()
+
+    assert crvusd.balanceOf(rewards_handler) == 0
+
+    # TODO more assertions to validate internal vault logic
+    # This probably requires boa eval for vvm.
+
+
+def test_over_time(rewards_handler):
+    with boa.reverts("rewards should be distributed over time"):
+        rewards_handler.process_rewards()
+
+
+def test_no_rewards(rewards_handler, curve_dao):
+    rewards_handler.set_distribution_time(1234, sender=curve_dao)
+
+    with boa.reverts("no rewards to distribute"):
+        rewards_handler.process_rewards()

tests/unitary/rewards_handler/test_recover_erc20.py

@@ -7,17 +7,24 @@ def mock_erc20():
     return boa.load("tests/mocks/MockERC20.vy")
 
 
-def test_default_behavior(rewards_handler, mock_erc20, curve_dao):
+@pytest.fixture(scope="module")
+def recovery_manager(rewards_handler, curve_dao):
+    _recovery_admin = boa.env.generate_address()
+    rewards_handler.grantRole(rewards_handler.RECOVERY_MANAGER(), _recovery_admin, sender=curve_dao)
+    return _recovery_admin
+
+
+def test_default_behavior(rewards_handler, mock_erc20, recovery_manager):
     rescue_address = boa.env.generate_address()
     boa.deal(mock_erc20, rewards_handler, 10**18)
-    rewards_handler.recover_erc20(mock_erc20, rescue_address, sender=curve_dao)
+    rewards_handler.recover_erc20(mock_erc20, rescue_address, sender=recovery_manager)
     assert mock_erc20.balanceOf(rescue_address) == 10**18
     assert mock_erc20.balanceOf(rewards_handler) == 0
 
 
-def test_crvusd_not_allowed(rewards_handler, crvusd, curve_dao):
+def test_crvusd_not_allowed(rewards_handler, crvusd, recovery_manager):
     with boa.reverts("can't recover crvusd"):
-        rewards_handler.recover_erc20(crvusd, boa.env.generate_address(), sender=curve_dao)
+        rewards_handler.recover_erc20(crvusd, boa.env.generate_address(), sender=recovery_manager)
 
 
 def test_role_access(rewards_handler):