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

Soloseng/celo-minting-schedule #10995

Open
wants to merge 59 commits into
base: master
Choose a base branch
from
Open

Conversation

soloseng
Copy link
Contributor

@soloseng soloseng commented May 14, 2024

Description

Created a contract MintGoldSchedule.sol that calculates the amount of CELO to mint and distribute to the community fund and carbon offsetting fund address. This contract can be deployed on L1, but will only be activated on L2 by calling MintGoldSchedule.setDependencies().

This schedule is based on the existing schedule previously defined by EpochRewards contract. In this new schedule, only the percentage of CELO allocated to community rewards and carbon offsetting fund is minted.

As with the L1 minting schedule, the L2 minting schedule in only defined for the first 15 years after L1 genesis block. Future modification to the MintGoldSchedule contract will be required to define a schedule beyond the first 15 years. Additionally, setting the distribution fraction for any fund is prohibited once the first 15 years have elapsed.

GoldToken.sol was updated to only allow minting by the owner or the MintGoldSchedule contract.

Other changes

  • IGoldToken.sol interface was added to allow MintGoldSchedule to call GoldToken.mint() .
  • A new MockGovernance contract, using pragma 0.8, was added to allow testing of the MintGoldSchedule. Using deployCodeTo("Governance.sol", abi.encode(false), governanceAddress); does not work, as the Governance contract bytecode has a placeholder for the libraries that it is using and Foundry does not provide a way to link the library during the compilation step of the tests.

Tested

  • Added unit tests for this new contract.
  • Updated goldToken test to addd minting tests.

Related issues

@soloseng soloseng marked this pull request as ready for review May 17, 2024 00:30
Copy link
Contributor

@mcortesi mcortesi left a comment

Choose a reason for hiding this comment

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

left some comments, still needing to review the actual math

packages/protocol/contracts/common/GoldToken.sol Outdated Show resolved Hide resolved
packages/protocol/migrations_sol/Migration.s.sol Outdated Show resolved Hide resolved
uint256 _communityRewardFraction,
address _carbonOffsettingPartner,
uint256 _carbonOffsettingFraction,
address registryAddress
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't registry address a well known constant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, however it has been common practice to include the registry address as an initialze variable rather that hardcode it.

removed, since in this case initialize() no longer takes args.

areDependenciesSet = true;
l2StartTime = _l2StartTime;
setRegistry(registryAddress);
communityRewardFund = address(getGovernance());
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

@soloseng soloseng May 29, 2024

Choose a reason for hiding this comment

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

same as above, this is to ensure that the community reward fund address is the correct one for L2

setRegistry(registryAddress);
communityRewardFund = address(getGovernance());
totalSupplyAtL2Start = getGoldToken().totalSupply();
setCommunityRewardFraction(_communityRewardFraction);
Copy link
Contributor

Choose a reason for hiding this comment

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

we could get this from the epoch rewards contract to make it clear that we're not using a new value?

Copy link
Contributor Author

@soloseng soloseng May 29, 2024

Choose a reason for hiding this comment

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

This will. To avoid importing the EpochRewards contract (not included in registry) into this one just to read the fraction variable, the fractions are passed in as a parameter. There should be minimal confusion, as the function parameters will be defined in the governance proposal that executes this function.

Also opted not to hard code the values, as these values could be changed at anytime before deployment by governance proposal, such as this one.

So it is best to let the governance proposal set the fraction in case it is later decided that it should change back to 25%

communityRewardFund = address(getGovernance());
totalSupplyAtL2Start = getGoldToken().totalSupply();
setCommunityRewardFraction(_communityRewardFraction);
setCarbonOffsettingFund(_carbonOffsettingPartner, _carbonOffsettingFraction);
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above

* @param _carbonOffsettingFraction The percentage of rewards going to carbon offsetting partner.
* @param registryAddress Address of the deployed contracts registry.
*/
function setDependecies(
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe called activate and just set the l2StartTime and totalSupplyAtCall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the name as suggested, but left the other setters to avoid misconfigs during L2 transition.

* @return communityTargetRewards The community reward that can be minted according to the target schedule.
* @return carbonFundTargetRewards The carbon offsetting reward that can be minted according to the target schedule.
*/
function getTargetGoldTotalSupply()
Copy link
Contributor

Choose a reason for hiding this comment

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

how should i review this? same logic is in epoch rewards?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the most part, same logic.

The only difference, is that it's not minting the fraction that would go to validators. This results in less CELO being minted in the first 15 year period than originally planned for.

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.

None yet

3 participants