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

Pip 26 #58

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

Pip 26 #58

wants to merge 11 commits into from

Conversation

simonDos
Copy link

@simonDos simonDos commented May 8, 2024

With this PR, the overall emission rate is lowered from 3% to 2.5%
This is done in order to lower the share going to the StakeManager from 2% to 1.5%
as was planned in PIP-26.

To achieve this, the DefaultEmissionManagerProxy will have its implementation upgraded to 1.2.0

In the new implementation, INTEREST_PER_YEAR_LOG2 is set to a new value representing log2(1.025),
startTimestamp is reset to the time of reinitialization and a new start supply (START_SUPPLY_1_2_0) public storage variable is introduced, because the old one is a constant.

The new emission rate goes into effect as soon as the proxy has been updated an reinitialized.

(note: setting a new treasury address (Community Governance) will also be part of the 1.2.0 upgrade and can be seen tested here, but will be handled in a different PR)

@simonDos simonDos requested a review from a team as a code owner May 8, 2024 09:36
Copy link

@web3security web3security left a comment

Choose a reason for hiding this comment

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

It is encouraged to test the real scenario beyond unit tests, that is, forking the real environment as it is today and compare both scenarios with and without the upgrade/change. For that we need to time-travel (i.e. each 2 weeks - which is the usual minting cadence) to see real impact and assert the expected values along the time.

src/DefaultEmissionManager.sol Show resolved Hide resolved
src/DefaultEmissionManager.sol Show resolved Hide resolved
src/DefaultEmissionManager.sol Show resolved Hide resolved
src/DefaultEmissionManager.sol Show resolved Hide resolved
src/DefaultEmissionManager.sol Outdated Show resolved Hide resolved
@@ -224,6 +226,6 @@ contract DefaultEmissionManagerTest is Test {
inputs[2] = vm.toString(delay);
inputs[3] = vm.toString(polygon.totalSupply());
uint256 newSupply = abi.decode(vm.ffi(inputs), (uint256));
assertApproxEqAbs(newSupply, emissionManager.inflatedSupplyAfter(block.timestamp + delay), 1e20);

Choose a reason for hiding this comment

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

Some assertions have been removed without further explanation like removing += by = as well as block.timestamp dependencies which is not good as we are just testing within a block now (not recommended for critical production env)

Copy link
Author

Choose a reason for hiding this comment

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

I added this comment in line 200:

// now that we actually pass this to calc.js, we only need to set it once.
        uint256 initialTotalSupply = polygon.totalSupply();

before, calc.js had startSupply hardcoded at 10_000_000_000e18
now that our value actually gets parsed, we only set initialTotalSupply one time at the beginning.
This leads to us also not having to increment the balance and stakeManagerBalance anymore, but simply setting them to the new value, because when we calculate totalAmtMinted, initialTotalSupply now stays the same. Hope that makes sense :)

Choose a reason for hiding this comment

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

it makes sense, thanks Simon.

Copy link

sonarcloud bot commented Jun 7, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

@web3security web3security left a comment

Choose a reason for hiding this comment

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

Thanks!

/// @custom:security-contact security@polygon.technology
contract DefaultEmissionManager is Ownable2StepUpgradeable, IDefaultEmissionManager {
using SafeERC20 for IPolygonEcosystemToken;

uint256 public constant INTEREST_PER_YEAR_LOG2 = 0.04264433740849372e18;
uint256 public constant INTEREST_PER_YEAR_LOG2 = 0.03562390973072122e18; // log2(1.025)

Choose a reason for hiding this comment

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

Just a comment about accuracy, it seems log2(1.025) = 0,0356239097307213452956518780335185248288369925056043873955

so maybe we can use 0.03562390973072135e18 wdyt? How the number was calculated?

@@ -224,6 +226,6 @@ contract DefaultEmissionManagerTest is Test {
inputs[2] = vm.toString(delay);
inputs[3] = vm.toString(polygon.totalSupply());
uint256 newSupply = abi.decode(vm.ffi(inputs), (uint256));
assertApproxEqAbs(newSupply, emissionManager.inflatedSupplyAfter(block.timestamp + delay), 1e20);

Choose a reason for hiding this comment

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

it makes sense, thanks Simon.

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