Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds backwards compatibility in UsingPrecompile contract. #11236

Conversation

soloseng
Copy link
Contributor

@soloseng soloseng commented Oct 4, 2024

Description

Updated UsingPrecompile contract to maintain backwards compatibility after L2 migration.

Fucntions that maintained backwards compatibility query the required information from the EpochManager contract instead of the precompiles.

  • Remove onlyL1 in validators.sol

Other changes

Was required to add the list of elected validators to the Epoch struct of the EpochManager contract to keep the historical list of elected validators at a given epoch number.

Tested

Added unit test.

Related issues

Backwards compatibility

for some select functions

@soloseng soloseng requested a review from a team as a code owner October 4, 2024 21:32
Copy link

openzeppelin-code bot commented Oct 4, 2024

Adds backwards compatibility in UsingPrecompile contract.

Generated at commit: 50bec5c8a89c6e7d9026818ef1d591cf0e891b53

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
2
0
14
43
62
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

@soloseng soloseng self-assigned this Oct 4, 2024
@soloseng
Copy link
Contributor Author

soloseng commented Oct 4, 2024

As of 50bec5c
I was able to add the required functions to preserve backwards compatibility to the UsingRegistry contract.

Testing is not fully implemented for registry functions, but the plan was to use EpochManager_WithMock.sol for testing needs and if possible remove MockEpochManager.sol

stCELO only uses numberValidatorsInCurrentSet ; validatorSignerAddressFromCurrentSet but I also added compatibility for the functions that perform the same query using specific blocknumber incase other contracts in the CELo ecosystem use them.

To get the epoch info based on block number, I used binary search to avoid looping through all the epoch 1x1. This was required for getEpochNumberOfBlock(); validatorSignerAddressFromSet()

We could further improve the compute cost by introducing a mapping of blocksToEpoch,

mapping(uint256 => uint256) internal blockToEpoch;

but that would be a more complex change and increase the gas cost of processing an epoch as we'd have to store this info during processing.

NOTE: Not all functions have backwards compatibility.

Copy link
Contributor

@m-chrzan m-chrzan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but there's a few, potentially very impactful, gas optimizations possible. There's no need to read the entire elected array into memory when we only need one field of the Epoch struct, or just one of the elected addresses. These should be fairly simple changes that would reduce the number of SLOADs by a factor of ~100 where relevant.

packages/protocol/contracts-0.8/common/EpochManager.sol Outdated Show resolved Hide resolved
packages/protocol/contracts-0.8/common/EpochManager.sol Outdated Show resolved Hide resolved
packages/protocol/contracts/common/UsingPrecompiles.sol Outdated Show resolved Hide resolved
packages/protocol/test-sol/utils08.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@martinvol martinvol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to fix all precompiles in all contracts

@pahor167 pahor167 enabled auto-merge (squash) October 15, 2024 08:36
@pahor167 pahor167 merged commit 7566b18 into release/core-contracts/12 Oct 15, 2024
40 of 42 checks passed
@pahor167 pahor167 deleted the soloseng/UsingRegistry-backward-compatibility-fix branch October 15, 2024 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants