[DO NOT MERGE] Epoch manager WIP #11189
Conversation
no working test. Co-authored-by: Martín Volpe <[email protected]>
…o into feat/epoch-manager
* 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]>
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
…lo-monorepo into feat/l2-epoch-system
* 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 ∆
…lo-monorepo into feat/l2-epoch-system
| address[] calldata groups, | ||
| address[] calldata lessers, | ||
| address[] calldata greaters |
There was a problem hiding this comment.
An optimization to have less calldata is to use the index in a list of validators rather than passing the addresses. Would that make sense?
There was a problem hiding this comment.
we agreed to leave this for the end, as it will potentially take more gas
* ++ contract function * ++ comment
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat are GitHub dependencies?Contains a dependency which resolves to a GitHub URL. Dependencies fetched from GitHub specifiers are not immutable can be used to inject untrusted code or reduce the likelihood of a reproducible install. Publish the GitHub dependency to npm or a private package repository and consume it from there. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
[DO NOT MERGE] Epoch manager draft
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
martinvol
changed the title
* 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
* 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
| import "@openzeppelin/contracts8/access/Ownable.sol"; | ||
|
|
||
| contract ScoreManager is Initializable, Ownable { | ||
| mapping(address => uint256) public scores; |
There was a problem hiding this comment.
i would have 'validatorScores' and 'groupScores'
It's just weird to do setValidatorScore(0x1, someScore) and then do getGroupScore(0x01) and get that score.
There was a problem hiding this comment.
I would actually abstract this, you set scores of addresses, doesn't really matter if it's a group or a validator.
In fact, we have logic to define the score of a group from the score of its members in Validators.calculateGroupEpochScore. This function is no longer used because it receives uptimes as parameters, but the logic that I paste could be reused:
for (uint256 i = 0; i < uptimes.length; i = i.add(1)) {
sum = sum.add(FixidityLib.wrap(calculateEpochScore(uptimes[i])));
}
return sum.divide(FixidityLib.newFixed(uptimes.length)).unwrap();
Then we just check if the score manager overwrote that, or the average of its members should be used.
There was a problem hiding this comment.
I'm ok with having a getScore(address) and then just a scores mapping. Or having getValidatorScore and getGroupScore and then 2 mappings. Either way
| return (1, 1, 0, 0); | ||
| } | ||
|
|
||
| function _getFirstBlockOfEpoch(uint256 currentEpoch) internal view returns (uint256) { |
There was a problem hiding this comment.
many ways to do this, there's a previous comment for this. please change
There was a problem hiding this comment.
this is a pending known issue I'll work on tomorow
|
|
||
| for (uint256 i = 0; i < numberElectedValidators; i++) { | ||
| // TODO: document how much gas this takes for 110 signers | ||
| address validatorAccountAddress = getAccounts().validatorSignerToAccount( |
There was a problem hiding this comment.
cache getAccounts() in a variable, to avoid fetching it each time
| */ | ||
| function release(address to, uint256 amount) external onlyEpochManager { | ||
| require(address(this).balance >= amount, "Insufficient balance."); | ||
| require(getCeloToken().transfer(to, amount), "CELO transfer failed."); |
There was a problem hiding this comment.
any reason why we prefer doing an ERC20 transfer, instead of a native transfer. My rule of thumb is that Token Duality is only for cases where you NEED a ERC20, for the rest of use cases we should just use native transfer. They are much cheaper gas wide
There was a problem hiding this comment.
It's nice to have the event log and the checks the transfer function has, as rule of thumb I always use the ERC20 transfer 🤔
There was a problem hiding this comment.
yo do have an event here. The Release event.
|
|
||
| modifier onlyEpochManagerEnabler() { | ||
| require( | ||
| msg.sender == registry.getAddressForOrDie(EPOCH_MANAGER_ENABLER_REGISTRY_ID), |
There was a problem hiding this comment.
I'm against having a registry entry for the epoch manager enabler. This is too much complexity for something that it's just a one time only contract. We can just inject the enable address at initialize, and call it a day
There was a problem hiding this comment.
Not sure why you say its complex. Our current tooling already adds the contract to the registry when its deployed.
If we were to pass the epochManagerEnabler address during initialization of the epochManager, we would have to modify our deployment tooling, or have to deploy in two phases just to add this address. That seems more complex then just fetching from the registry.
Could you elaborate more on the complexity you see here?
There was a problem hiding this comment.
What i mean is that you're adding a registry entry (that lives forever) for a smart contract that it's basically a script that is only applied once during migration.
That sound "smelly". For a script that i'm only using once, i want to forget about it the moment i'm done using it. Instead, in 2 years, you'll see the registry and there it is. And then on all mappings of registry entries you'll have it.
So, you're choosing something that it's easy now, in expense of keeping garbage for the whole history of the celo chain.
There was a problem hiding this comment.
I agree with seb, it does have more moving pieces but saves having to do follow-up calls to the contract after it was deployed.
There was a problem hiding this comment.
primitives team decision. My take still the same, leaving trails and living objects feels smelly to me. Less pain today in exchange for more pain tomorrow
| * @notice Allows the EpochManager contract to mint stable token for itself. | ||
| * @param amount The amount to be minted. | ||
| */ | ||
| function mintStableToEpochManager( |
There was a problem hiding this comment.
as said earlier i think this is unnecessary delegation just have this logic on the epoch manager
There was a problem hiding this comment.
The original approach was to have the logic in epochManager , but for this to work, we would need mento to update their contract to allow epochManager to mint. Since they have a different deployment/development process, this would add more time to release.
There was a problem hiding this comment.
ok, then please add a comment here, explaining this, so we can change it in the future
| @@ -1325,11 +1420,17 @@ contract Validators is | |||
| */ | |||
| function updateMembershipHistory(address account, address group) private returns (bool) { | |||
| MembershipHistory storage history = validators[account].membershipHistory; | |||
| uint256 epochNumber = getEpochNumber(); | |||
| uint256 epochNumber; | |||
| // SPDX-License-Identifier: LGPL-3.0-only | ||
| pragma solidity >=0.5.13 <0.9.0; | ||
|
|
||
| interface IEpochManager { |
There was a problem hiding this comment.
review methods here, i don't see all of them listed
| // SPDX-License-Identifier: LGPL-3.0-only | ||
| pragma solidity >=0.5.13 <0.9.0; | ||
|
|
||
| interface IEpochManagerEnabler { |
There was a problem hiding this comment.
unnecessary to have an interface for this
There was a problem hiding this comment.
This is used for forge testing and migration scripts.
| // SPDX-License-Identifier: LGPL-3.0-only | ||
| pragma solidity >=0.5.13 <0.9.0; | ||
|
|
||
| interface IScoreManager { |
There was a problem hiding this comment.
as said in some other PR. A Score manager has getters for scores and only that. Setters are an implemetation detail, you can create another socre manager that computes scores some other way and setters wouldn't make sense there.
Instead just have some IScoreManager or IScoreREader, and that's the dependency EpochManager needs
There was a problem hiding this comment.
in this case maybe move to another interface IScoreSetter or maybe for now not needed at all.
* 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 when sending
| * delegation beneficiary. | ||
| * @param validator Account of the validator. | ||
| */ | ||
| function sendValidatorPayment(address validator) external { |
There was a problem hiding this comment.
We could, for an extra extra sanity check, but we're only interacting with other core contracts and not doing any native transfers.
There was a problem hiding this comment.
Mento is not a core contract anymore
| .fromFixed(); | ||
| uint256 validatorPayment = remainingPayment.fromFixed() - delegatedPayment; | ||
|
|
||
| IStableToken stableToken = IStableToken(getStableToken()); |
There was a problem hiding this comment.
I think we should use IER20 here to decouple
Placeholder PR to have the CI