Send validator payment on actions that would modify reward distribution #11211

m-chrzan · 2024-09-18T19:57:42Z

Description

Some functions would modify the amount or payment split in EpochManager.sendValidatorPayment. This PR adds a call to sendValidatorPayment before such modifications occur to ensure the payment split is deterministic after epoch processing.

The functions are:

Validators._deaffiliate (called by affiliate, deaffiliate, and forceDeaffiliateIfValidator), which would change the group the validator is associated with, thus the group that would receive the group portion of the payment
Validators.updateCommission, which would change the split between validators and group.
Accounts.setPaymentDelegation, which would change the split between validator and beneficiary (and/or who the beneficiary is). However, since this is a voluntary payment from the validator, the protocol does not need to ensure this stays the same, so we can choose not to use this mechanism here.

TODO:

Do this for Validators.setPaymentDelegation

Other changes

Removed allowOnlyL1 from a few functions
Function ordering for linter

Tested

Unit tests.

Related issues

Fixes https://github.com/celo-org/celo-blockchain-planning/issues/552

…ifications

m-chrzan · 2024-09-19T10:20:20Z

packages/protocol/contracts-0.8/common/EpochManager.sol

@@ -245,6 +245,60 @@ contract EpochManager is
    epochProcessing.status = EpochProcessStatus.NotStarted;
  }

+  /**


This can be ignored in review, just a function ordering change.

m-chrzan · 2024-09-19T10:20:42Z

packages/protocol/contracts-0.8/common/mocks/MockAccounts.sol

-    uint256 fraction
-  ) public {
-    delegations[validator] = PaymentDelegation(beneficiary, FixidityLib.wrap(fraction));
+  function setValidatorSigner(address account, address signer) external {


Ditto for everything in this file, function ordering for linter.

martinvol · 2024-09-20T21:45:57Z

packages/protocol/contracts-0.8/governance/Validators.sol

@@ -1497,6 +1498,17 @@ contract Validators is
    return true;
  }

+  function _sendValidatorPaymentIfNecessary(address validator) private {
+    getEpochManager().sendValidatorPayment(validator);


should this avoid the call completely in L1?

are we sure this actually doesn't revert on L1?

sendValidatorPayment should probably revert if the epoch system is not initialized

…epo into m-chrzan/send-payment-on-modifications

…ifications

packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol

soloseng

Left a few comments

soloseng · 2024-09-23T14:28:31Z

packages/protocol/contracts-0.8/common/EpochManager.sol

@@ -107,6 +107,11 @@ contract EpochManager is
   */
  constructor(bool test) public Initializable(test) {}

+  modifier onlySystemAlreadyInitialized() {


NIT: function ordering

soloseng · 2024-09-23T14:33:36Z

packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol

+contract IntegrationsValidators is Test, Devchain {
+  function test_deaffiliateWorskWithEpochManager() public {
+    // address of an allowed validator
+    vm.prank(address(0x15d34AAf54267DB7D7c367839AAf71A00a2C6A65));


How are we getting this address? could we have a call to validators contract to get this address instead of hardcoding it?

soloseng · 2024-09-23T17:01:55Z

packages/protocol/contracts/governance/test/MockValidators.sol

@@ -84,11 +84,11 @@ contract MockValidators is IValidators, IsL2Check {
  }

  function halveSlashingMultiplier(address) external {
-    allowOnlyL1();
+    allowOnlyL1(); // TODO remove


address todo

soloseng · 2024-09-23T17:05:18Z

packages/protocol/contracts-0.8/governance/Validators.sol

+    }
+  }
+
+  function _sendValidatorGroupPaymentsIfNecessary(ValidatorGroup storage group) private {


should there be an isL2 check in this function as well? this would avoid going thru the loop.

Alternatively, it could be implemented when the function is used.

* first compiling draft no working test. Co-authored-by: Martín Volpe <[email protected]> * Compiler fixes + finishNextEpochProcess * allocateValidatorsRewards compilable * First few tests * test fix * uisng registry instead * updated registry ID var names * typo * ++ mock contracts in 0.8 * ++ passing test using mock * Removal of using precompiles * ScoreManager refactor * Decouple epoch manager initializer from registry * isReadyToStartEpoch fix * startNextEpochProcess fixes * rename score manager to score reader * Rename transfer to release on CeloDistribution schedule * Celo distribution schedule renamed to CeloUnreleasedTreasure * conditions to getFirstBlockAtEpoch and getLastBlockAtEpoch * ScoreManager update * getFirstBlockOfEpoch in EpochManager initializer * extra checks * introduce new getValidatorsGroup * systemAlreadyInitialized update * Removal of IEpochManagerInterface * Made CI run on feature branch * merge fix * Added target for the CI * Fixed quote celo-monorepo.yml * Allow Validator registration in L2 without BLS key (#11181) * Allow validator registration in L2 without BLS key * Reallow ECDSA key change in L2 --------- Co-authored-by: Martín Volpe <[email protected]> * Fix CI * Fixed interfaces and imports * Build fix * lint * prettify * prettify 2 * Attempt to fix truffle build and migrations l2 epoch (#11190) * Added Blocking to LockedGold, Election (#11186) * add EpochManager to abis * force deploy abis * build fix * Move Validators.sol to 0.8 (#11192) * WIP * Validator test WIP, forge doesn't compile yet * buildable * most of the foundry tests working * Validator related tests fixed * truffle build working * cache bump * lint + prettify+ migrations * Enable optimization for Solidity 0.8 * prettify * Ci bump * CI bump 2 * Validators to 0.8 config * CI bump * foundry fix * Truffle migrations are partly fixed * Added import for ValidatorsMock08 in EpochManager.t.sol, I think it was removed by mistake * Added yarn.lock * Changes to mock with full implementation * Attempt to fix linking libraries not working with deployCodeTo foundry-rs/foundry#4049 * truffle migrations fixed * CI bump * lint * forge test fixes * artifacts test fix * lint * update of foundry version * add ProxyFactory import to tests * library linking fix * Foundry migrations fix * migration tests fix * CI bump * Little cleanup + retrigger CI * forgot to commit Validators.sol * Fixed the ABI encoded * lint * Fix contract versions * add Adapter to ignored contracts * revert of ReentrancyGuard change * lint fix * remove adapters from check * storage layout fix --------- Co-authored-by: pahor167 <[email protected]> * force release of canary abi * force new canary version * npm tag revert * Soloseng/update-l2-getepochnumber-logic (#11195) * disable getEpochSize on L2 * update registry * update relevant interfaces * update contracts with L2 `getEpochNumber()` logic * update tests with L2 `getEpochNumber()` logic * ++ TODO * moved constants to constants file * updated allocated supply function to handle L1 & L2 cases * made `epochManager.currenEpochNumber()` private, to avoid returning 0 when not initialized. * PR feedback * Passing validators test using mockEpochManager for L2 tests * clean up * fixed other failing tests * using mockEpochManager instead of interface. Fixed failing tests. * happy linter * revert npm tag ∆ * ++ TODO and comment * add score manager to abis * Split initEpochManager Function (#11199) * ++ contract function * ++ comment * startNextEpochProcess unit & integration test (#11191) * unit test with mocks * ++ integration tests * clean up * -- logging * removed duplicate interface * using `MockCeloToken` to get test to pass. Fails when it hits a precompile in `EpochRewards.sol` * removed endEpochTimestamp * moved IEpochManager to 0.5 folder * added L2 conditions for EpochRewards functions using precompiles Still missing tests * renamed EpochManagerInitializer due to name conflict * ++ more unit test * setup anvil migration fix name conflict * compiles * ++ require fund in unreleased treasury * Updated regex * ++ registry 0.8 for testing only * clean up * ++ unit test * initial integration test using L1 devchain * ++ comment * -- forge based integration test * ++ to const * happy linter * update contract name * ++ PR feedback * ++ checks * updated carbon address * proxy stableToken mint call via Validators contract * -- duplicate imports * removed registry08. replaced with vm call * PR feedback * -- coment * passing unit tests * clean up * ++ mintStable test * -- TODO; compiles test when filtering * PR feedback * updated migration script to add more validators * passing integration test * removed test for zero amount * yarn build fix * clean up comments && TODO * revert change as out of scope * E2E EpochManager test + Epoch truffle migrations & Anvil L2 migration build fix (#11198) * Soloseng/dynamically-fetch-epochmanagerenabler-address (#11207) * dynamically fetch epochManagerEnabler && carbonOffsettingPartner ++ to registry * PR feedback * removed onlyL1 modifier in setter functions * updated unit test to reflect changes * fixing tests * fix test * fixed migration data * fixed migration script error * removed unused modifier * removed duplicate or unused code * Implement sending of allocated validator payments (#11197) * EpochManager fixes (#11208) * truffle build fix * build fix * PR comments * prettify * rename of registerValidator overload * bug fix * extensing epochManager e2e test * yarn lint * Reset pending payment to 0 after sending (#11209) Reset pending payment to 0 when sending * Deleted duplicated import * Make captureEpochAndValidators work in constant time (#11210) * Use more general interface for token transfer (#11216) * Epoch manager enabler tests (#11213) * ++ basic test * -- celoToken balance check * cleanup comments * use celoToken instead of native token for `initializeSystem` balance check * ++ more test * removed additional epochs * Make sendValidatorPayment nonReentrant (#11217) * Martinvol/return account instead of signer (#11215) * EpochManager e2e tests add/remove validators between ecpochs (#11214) * Send validator payment on actions that would modify reward distribution (#11211) * Rename `CeloUnreleasedTreasure` to `CeloUnreleasedTreasury` (#11220) * updated `CeloUnrealeasedTreasure` to `CeloUnreleasedTreasury` * update `CeloUnreleasedTreasury` initial balance to use defined constant * renamed file * prevent CELO transfers to `CeloUnreleasedTreasury` * ++ comment * unused import * using internal accounting for released amount * updated integration test * PR feedback * PR feedback * reverted due to gas consideration * more revert --------- Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: pahor167 <[email protected]> Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: Martín Volpe <[email protected]> Co-authored-by: Martin <[email protected]> Co-authored-by: pahor167 <[email protected]>

Send validator payment on actions that cause deaffiliation

788bf19

m-chrzan requested a review from a team as a code owner September 18, 2024 19:57

m-chrzan marked this pull request as draft September 18, 2024 19:57

m-chrzan added 7 commits September 19, 2024 11:18

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-payment-on-mod…

97ebb0c

…ifications

Make commission update functions callable in L2

ff46ea3

Send validator payments to group when commission updated

ac3285f

Rename helper functions

53f9531

Remove debug logging and use individual pranks

1b82120

Change test names

6c965c7

Reorder functions to appease the linter

77544d7

m-chrzan changed the title ~~Send validator payment on actions that cause deaffiliation~~ Send validator payment on actions that would modify reward distribution Sep 19, 2024

m-chrzan marked this pull request as ready for review September 19, 2024 10:19

m-chrzan commented Sep 19, 2024

View reviewed changes

martinvol reviewed Sep 20, 2024

View reviewed changes

martinvol added 8 commits September 23, 2024 14:12

Merge branch 'feat/l2-epoch-system' of github.com:celo-org/celo-monor…

d3b8ecb

…epo into m-chrzan/send-payment-on-modifications

rolled back accidentaly lost in merge

cd93e0b

Added test and avoid calling on L1

09fd206

WIP

605325d

Merge branch 'feat/l2-epoch-system' into m-chrzan/send-payment-on-mod…

e8d2bab

…ifications

Don't try to send payments on L1

369774d

Fixed tests file

71b9f2b

Fix anvil test

0b49bd0

martinvol reviewed Sep 23, 2024

View reviewed changes

packages/protocol/test-sol/devchain/migration/IntegrationValidators.t.sol Outdated Show resolved Hide resolved

Query addresses from contracts

b975aac

soloseng approved these changes Sep 23, 2024

View reviewed changes

martinvol merged commit 02454f4 into feat/l2-epoch-system Sep 23, 2024
21 checks passed

martinvol deleted the m-chrzan/send-payment-on-modifications branch September 23, 2024 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send validator payment on actions that would modify reward distribution #11211

Send validator payment on actions that would modify reward distribution #11211

m-chrzan commented Sep 18, 2024 •

edited

Loading

m-chrzan Sep 19, 2024

m-chrzan Sep 19, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading

soloseng left a comment

soloseng Sep 23, 2024

soloseng Sep 23, 2024

soloseng Sep 23, 2024

soloseng Sep 23, 2024

Send validator payment on actions that would modify reward distribution #11211

Send validator payment on actions that would modify reward distribution #11211

Conversation

m-chrzan commented Sep 18, 2024 • edited Loading

Description

Other changes

Tested

Related issues

m-chrzan Sep 19, 2024

Choose a reason for hiding this comment

m-chrzan Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

martinvol Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

soloseng left a comment

Choose a reason for hiding this comment

soloseng Sep 23, 2024

Choose a reason for hiding this comment

soloseng Sep 23, 2024

Choose a reason for hiding this comment

soloseng Sep 23, 2024

Choose a reason for hiding this comment

soloseng Sep 23, 2024

Choose a reason for hiding this comment

m-chrzan commented Sep 18, 2024 •

edited

Loading

m-chrzan Sep 19, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading

martinvol Sep 20, 2024 •

edited

Loading