From 02454f4aebc4081b45cb72dd175e535057ab3d5b Mon Sep 17 00:00:00 2001 From: Martin Date: Mon, 23 Sep 2024 19:16:24 +0200 Subject: [PATCH] Send validator payment on actions that would modify reward distribution (#11211) --- .../contracts-0.8/common/EpochManager.sol | 13 ++- .../common/mocks/MockAccounts.sol | 1 + .../common/test/MockEpochManager.sol | 6 ++ .../contracts-0.8/governance/Validators.sol | 18 +++- .../common/interfaces/IEpochManager.sol | 1 + .../governance/test/MockValidators.sol | 4 +- .../foundry/run_integration_tests_in_anvil.sh | 4 +- .../devchain/e2e/common/EpochManager.t.sol | 20 ++-- .../migration/IntegrationValidators.t.sol | 12 +++ .../test-sol/unit/common/EpochManager.t.sol | 3 + .../governance/validators/Validators.t.sol | 102 +++++++++++++++--- 11 files changed, 148 insertions(+), 36 deletions(-) create mode 100644 packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol diff --git a/packages/protocol/contracts-0.8/common/EpochManager.sol b/packages/protocol/contracts-0.8/common/EpochManager.sol index 6a5dea9b221..fa5300e1213 100644 --- a/packages/protocol/contracts-0.8/common/EpochManager.sol +++ b/packages/protocol/contracts-0.8/common/EpochManager.sol @@ -107,6 +107,11 @@ contract EpochManager is */ constructor(bool test) public Initializable(test) {} + modifier onlySystemAlreadyInitialized() { + require(systemAlreadyInitialized(), "Epoch system not initialized"); + _; + } + /** * @notice Used in place of the constructor to allow the contract to be upgradable via proxy. * @param registryAddress The address of the registry core smart contract. @@ -154,8 +159,7 @@ contract EpochManager is /// start next epoch process. /// it freezes the epochrewards at the time of execution, /// and starts the distribution of the rewards. - function startNextEpochProcess() external nonReentrant { - require(systemAlreadyInitialized(), "Epoch system not initialized"); + function startNextEpochProcess() external nonReentrant onlySystemAlreadyInitialized { require(isTimeForNextEpoch(), "Epoch is not ready to start"); require(!isOnEpochProcess(), "Epoch process is already started"); epochProcessing.status = EpochProcessStatus.Started; @@ -247,7 +251,7 @@ contract EpochManager is * delegation beneficiary. * @param validator Account of the validator. */ - function sendValidatorPayment(address validator) external { + function sendValidatorPayment(address validator) external onlySystemAlreadyInitialized { FixidityLib.Fraction memory totalPayment = FixidityLib.newFixed( validatorPendingPayments[validator] ); @@ -300,8 +304,7 @@ contract EpochManager is } /// returns the current epoch number. - function getCurrentEpochNumber() external view returns (uint256) { - require(systemAlreadyInitialized(), "EpochManager system not yet initialized."); + function getCurrentEpochNumber() external view onlySystemAlreadyInitialized returns (uint256) { return currentEpochNumber; } diff --git a/packages/protocol/contracts-0.8/common/mocks/MockAccounts.sol b/packages/protocol/contracts-0.8/common/mocks/MockAccounts.sol index a28357f7246..51ead07c279 100644 --- a/packages/protocol/contracts-0.8/common/mocks/MockAccounts.sol +++ b/packages/protocol/contracts-0.8/common/mocks/MockAccounts.sol @@ -36,6 +36,7 @@ contract MockAccounts { ) public { delegations[validator] = PaymentDelegation(beneficiary, FixidityLib.wrap(fraction)); } + function deletePaymentDelegationFor(address validator) public { delete delegations[validator]; } diff --git a/packages/protocol/contracts-0.8/common/test/MockEpochManager.sol b/packages/protocol/contracts-0.8/common/test/MockEpochManager.sol index c4cb4d2e8ce..a6feae3109d 100644 --- a/packages/protocol/contracts-0.8/common/test/MockEpochManager.sol +++ b/packages/protocol/contracts-0.8/common/test/MockEpochManager.sol @@ -26,6 +26,8 @@ contract MockEpochManager is IEpochManager { bool initialized; mapping(uint256 => Epoch) private epochs; + event SendValidatorPaymentCalled(address validator); + function setCurrentEpochNumber(uint256 _newEpochNumber) external { currentEpochNumber = _newEpochNumber; } @@ -75,4 +77,8 @@ contract MockEpochManager is IEpochManager { { return (0, 0, 0, 0, 0); } + + function sendValidatorPayment(address validator) public { + emit SendValidatorPaymentCalled(validator); + } } diff --git a/packages/protocol/contracts-0.8/governance/Validators.sol b/packages/protocol/contracts-0.8/governance/Validators.sol index 0b3d2d7b498..4c526f9c62a 100644 --- a/packages/protocol/contracts-0.8/governance/Validators.sol +++ b/packages/protocol/contracts-0.8/governance/Validators.sol @@ -576,7 +576,6 @@ contract Validators is * payments made to its members. Must be in the range [0, 1.0]. */ function setNextCommissionUpdate(uint256 commission) external { - allowOnlyL1(); address account = getAccounts().validatorSignerToAccount(msg.sender); require(isValidatorGroup(account), "Not a validator group"); ValidatorGroup storage group = groups[account]; @@ -592,11 +591,12 @@ contract Validators is * @notice Updates a validator group's commission based on the previously queued update */ function updateCommission() external { - allowOnlyL1(); address account = getAccounts().validatorSignerToAccount(msg.sender); require(isValidatorGroup(account), "Not a validator group"); ValidatorGroup storage group = groups[account]; + _sendValidatorGroupPaymentsIfNecessary(group); + require(group.nextCommissionBlock != 0, "No commission update queued"); require(group.nextCommissionBlock <= block.number, "Can't apply commission update yet"); @@ -1498,6 +1498,7 @@ contract Validators is Validator storage validator, address validatorAccount ) private returns (bool) { + _sendValidatorPaymentIfNecessary(validatorAccount); address affiliation = validator.affiliation; ValidatorGroup storage group = groups[affiliation]; if (group.members.contains(validatorAccount)) { @@ -1508,6 +1509,19 @@ contract Validators is return true; } + function _sendValidatorPaymentIfNecessary(address validator) private { + if (isL2()) { + getEpochManager().sendValidatorPayment(validator); + } + } + + function _sendValidatorGroupPaymentsIfNecessary(ValidatorGroup storage group) private { + address[] memory members = group.members.getKeys(); + for (uint256 i = 0; i < members.length; i++) { + _sendValidatorPaymentIfNecessary(members[i]); + } + } + /** * @notice Returns the epoch number. * @return Current epoch number. diff --git a/packages/protocol/contracts/common/interfaces/IEpochManager.sol b/packages/protocol/contracts/common/interfaces/IEpochManager.sol index 4d48466c7a3..f1eadf15b86 100644 --- a/packages/protocol/contracts/common/interfaces/IEpochManager.sol +++ b/packages/protocol/contracts/common/interfaces/IEpochManager.sol @@ -13,6 +13,7 @@ interface IEpochManager { address[] calldata lessers, address[] calldata greaters ) external; + function sendValidatorPayment(address) external; function getCurrentEpoch() external view returns (uint256, uint256, uint256, uint256); function getCurrentEpochNumber() external view returns (uint256); function getElected() external view returns (address[] memory); diff --git a/packages/protocol/contracts/governance/test/MockValidators.sol b/packages/protocol/contracts/governance/test/MockValidators.sol index cc04430bc9d..54a87bdd5a3 100644 --- a/packages/protocol/contracts/governance/test/MockValidators.sol +++ b/packages/protocol/contracts/governance/test/MockValidators.sol @@ -84,11 +84,11 @@ contract MockValidators is IValidators, IsL2Check { } function halveSlashingMultiplier(address) external { - allowOnlyL1(); + allowOnlyL1(); // TODO remove } function forceDeaffiliateIfValidator(address validator) external { - allowOnlyL1(); + allowOnlyL1(); // TODO remove } function getTopGroupValidators(address group, uint256 n) public view returns (address[] memory) { diff --git a/packages/protocol/scripts/foundry/run_integration_tests_in_anvil.sh b/packages/protocol/scripts/foundry/run_integration_tests_in_anvil.sh index 94e23de7489..bdcd06f4f09 100755 --- a/packages/protocol/scripts/foundry/run_integration_tests_in_anvil.sh +++ b/packages/protocol/scripts/foundry/run_integration_tests_in_anvil.sh @@ -10,9 +10,9 @@ source $PWD/scripts/foundry/create_and_migrate_anvil_devchain.sh # Run integration tests echo "Running integration tests..." -forge test \ +time FOUNDRY_PROFILE=devchain forge test \ -vvv \ ---match-contract RegistryIntegrationTest \ +--match-path "test-sol/devchain/migration/*" \ --fork-url $ANVIL_RPC_URL # Stop devchain diff --git a/packages/protocol/test-sol/devchain/e2e/common/EpochManager.t.sol b/packages/protocol/test-sol/devchain/e2e/common/EpochManager.t.sol index f18fd68c6f7..011b6948ab5 100644 --- a/packages/protocol/test-sol/devchain/e2e/common/EpochManager.t.sol +++ b/packages/protocol/test-sol/devchain/e2e/common/EpochManager.t.sol @@ -12,6 +12,16 @@ import "@test-sol/utils/ECDSAHelper08.sol"; import "@openzeppelin/contracts8/utils/structs/EnumerableSet.sol"; contract E2E_EpochManager is Test, Devchain, Utils08, ECDSAHelper08 { + struct VoterWithPK { + address voter; + uint256 privateKey; + } + + struct GroupWithVotes { + address group; + uint256 votes; + } + address epochManagerOwner; address epochManagerEnabler; address[] firstElected; @@ -24,16 +34,6 @@ contract E2E_EpochManager is Test, Devchain, Utils08, ECDSAHelper08 { uint256[] groupScore = [5e23, 7e23, 1e24, 4e23]; uint256[] validatorScore = [1e23, 1e23, 1e23, 1e23, 1e23, 1e23, 1e23]; - struct VoterWithPK { - address voter; - uint256 privateKey; - } - - struct GroupWithVotes { - address group; - uint256 votes; - } - mapping(address => uint256) addressToPrivateKeys; mapping(address => VoterWithPK) validatorToVoter; diff --git a/packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol b/packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol new file mode 100644 index 00000000000..2ba650dffe7 --- /dev/null +++ b/packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity >=0.8.7 <0.8.20; + +import "celo-foundry-8/Test.sol"; +import { Devchain } from "@test-sol/devchain/e2e/utils.sol"; + +contract IntegrationsValidators is Test, Devchain { + function test_deaffiliateWorskWithEpochManager() public { + vm.prank(election.electValidatorAccounts()[0]); + validators.deaffiliate(); + } +} diff --git a/packages/protocol/test-sol/unit/common/EpochManager.t.sol b/packages/protocol/test-sol/unit/common/EpochManager.t.sol index f4c5dfc5573..ba41104b2cf 100644 --- a/packages/protocol/test-sol/unit/common/EpochManager.t.sol +++ b/packages/protocol/test-sol/unit/common/EpochManager.t.sol @@ -288,6 +288,9 @@ contract EpochManagerTest_sendValidatorPayment is EpochManagerTest { members[1] = validator2; mockValidators.setMembers(group, members); + vm.prank(epochManagerEnabler); + epochManager.initializeSystem(firstEpochNumber, firstEpochBlock, firstElected); + stableToken.mint(address(epochManager), paymentAmount * 2); epochManagerBalanceBefore = stableToken.balanceOf(address(epochManager)); epochManager._setPaymentAllocation(validator1, paymentAmount); diff --git a/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol b/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol index 0d3f4f21ee2..15dd3b2ddc2 100644 --- a/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol +++ b/packages/protocol/test-sol/unit/governance/validators/Validators.t.sol @@ -138,6 +138,8 @@ contract ValidatorsTest is Test, TestConstants, Utils, ECDSAHelper { uint256 groupPayment ); + event SendValidatorPaymentCalled(address validator); + function setUp() public { owner = address(this); group = actor("group"); @@ -1412,6 +1414,21 @@ contract ValidatorsTest_Affiliate_WhenValidatorIsAlreadyAffiliatedWithValidatorG assertTrue(election.isIneligible(group)); } + + function test_ShouldNotTryToSendValidatorPayment_WhenL1() public { + vm.prank(validator); + validators.affiliate(group); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 0); + } + + function test_ShouldSendValidatorPayment_WhenL2() public { + _whenL2(); + vm.expectEmit(true, true, true, true); + emit SendValidatorPaymentCalled(validator); + vm.prank(validator); + validators.affiliate(group); + } } contract ValidatorsTest_Deaffiliate is ValidatorsTest { @@ -1537,6 +1554,21 @@ contract ValidatorsTest_Deaffiliate is ValidatorsTest { validators.deaffiliate(); assertTrue(election.isIneligible(group)); } + + function test_ShouldNotTryToSendValidatorPayment_WhenL1() public { + vm.prank(validator); + validators.affiliate(group); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 0); + } + + function test_ShouldSendValidatorPayment_WhenL2() public { + _whenL2(); + vm.expectEmit(true, true, true, true); + emit SendValidatorPaymentCalled(validator); + vm.prank(validator); + validators.deaffiliate(); + } } contract ValidatorsTest_UpdateEcdsaPublicKey is ValidatorsTest { @@ -2487,13 +2519,6 @@ contract ValidatorsTest_SetNextCommissionUpdate is ValidatorsTest { assertEq(_commission, commission.unwrap()); } - function test_Reverts_SetValidatorGroupCommission_WhenL2() public { - _whenL2(); - vm.prank(group); - vm.expectRevert("This method is no longer supported in L2."); - validators.setNextCommissionUpdate(newCommission); - } - function test_ShouldSetValidatorGroupNextCommission() public { vm.prank(group); validators.setNextCommissionUpdate(newCommission); @@ -2534,7 +2559,21 @@ contract ValidatorsTest_UpdateCommission is ValidatorsTest { function setUp() public { super.setUp(); - _registerValidatorGroupHelper(group, 1); + _registerValidatorGroupHelper(group, 2); + + _registerValidatorHelper(validator, validatorPk); + _registerValidatorHelper(otherValidator, otherValidatorPk); + + vm.prank(validator); + validators.affiliate(group); + (, , address _affiliation1, , ) = validators.getValidator(validator); + + vm.prank(otherValidator); + validators.affiliate(group); + (, , address _affiliation2, , ) = validators.getValidator(otherValidator); + + require(_affiliation1 == group, "Affiliation failed."); + require(_affiliation2 == group, "Affiliation failed."); } function test_ShouldSetValidatorGroupCommission() public { @@ -2551,13 +2590,6 @@ contract ValidatorsTest_UpdateCommission is ValidatorsTest { assertEq(_commission, newCommission); } - function test_Reverts_SetValidatorGroupCommission_WhenL2() public { - _whenL2(); - vm.prank(group); - vm.expectRevert("This method is no longer supported in L2."); - validators.setNextCommissionUpdate(newCommission); - } - function test_Emits_ValidatorGroupCommissionUpdated() public { vm.prank(group); validators.setNextCommissionUpdate(newCommission); @@ -2600,6 +2632,31 @@ contract ValidatorsTest_UpdateCommission is ValidatorsTest { vm.prank(group); validators.updateCommission(); } + + function test_ShouldNotTryTodSendMultipleValidatorPayments_WhenL1() public { + vm.prank(validator); + validators.affiliate(group); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 0); + } + + function test_ShouldSendMultipleValidatorPayments_WhenL2() public { + vm.prank(group); + validators.addFirstMember(validator, address(0), address(0)); + vm.prank(group); + validators.addMember(otherValidator); + vm.prank(group); + validators.setNextCommissionUpdate(newCommission); + blockTravel(commissionUpdateDelay); + + _whenL2(); + vm.expectEmit(true, true, true, true); + emit SendValidatorPaymentCalled(validator); + vm.expectEmit(true, true, true, true); + emit SendValidatorPaymentCalled(otherValidator); + vm.prank(group); + validators.updateCommission(); + } } contract ValidatorsTest_CalculateEpochScore is ValidatorsTest { @@ -3568,6 +3625,21 @@ contract ValidatorsTest_ForceDeaffiliateIfValidator is ValidatorsTest { vm.expectRevert("Only registered slasher can call"); validators.forceDeaffiliateIfValidator(validator); } + + function test_ShouldNotTryToSendValidatorPayment_WhenL1() public { + vm.prank(validator); + validators.affiliate(group); + Vm.Log[] memory entries = vm.getRecordedLogs(); + assertEq(entries.length, 0); + } + + function test_ShouldSendValidatorPayment_WhenL2() public { + _whenL2(); + vm.expectEmit(true, true, true, true); + emit SendValidatorPaymentCalled(validator); + vm.prank(paymentDelegatee); + validators.forceDeaffiliateIfValidator(validator); + } } contract ValidatorsTest_ForceDeaffiliateIfValidator_L2 is ValidatorsTest { function setUp() public {