From 886b8d0f4cf55666687f26cde0ba45e3d91f339b Mon Sep 17 00:00:00 2001 From: Alberto Date: Wed, 9 Oct 2024 13:56:29 +0200 Subject: [PATCH] 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 | 14 ++++++--- tests/unitary/conftest.py | 1 + .../rewards_handler/test_process_rewards.py | 29 +++++++++++++++++++ .../rewards_handler/test_recover_erc20.py | 15 +++++++--- 4 files changed, 51 insertions(+), 8 deletions(-) create mode 100644 tests/unitary/rewards_handler/test_process_rewards.py diff --git a/contracts/RewardsHandler.vy b/contracts/RewardsHandler.vy index 244d36f..d486a0b 100644 --- a/contracts/RewardsHandler.vy +++ b/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. diff --git a/tests/unitary/conftest.py b/tests/unitary/conftest.py index 0386374..8576b9d 100644 --- a/tests/unitary/conftest.py +++ b/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() diff --git a/tests/unitary/rewards_handler/test_process_rewards.py b/tests/unitary/rewards_handler/test_process_rewards.py new file mode 100644 index 0000000..2e22a14 --- /dev/null +++ b/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() diff --git a/tests/unitary/rewards_handler/test_recover_erc20.py b/tests/unitary/rewards_handler/test_recover_erc20.py index 636780d..0ccd3ee 100644 --- a/tests/unitary/rewards_handler/test_recover_erc20.py +++ b/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):