minter: Add inflation bounds to inflation adjustment algorithm #645

SidestreamSweatyPumpkin · 2025-03-26T19:29:53Z

What does this pull request do? Explain your changes. (required)

This PR extends the original PR (introduced inflation bounds for the Minter) with tests and other changes required for deploying the contract and merging the PR.

Specific updates (required)

Added unit tests for the maxInflation and minInflation constructor arguments, setters and the updated inflation logic
Added functionality to seamlessly transfer state of the old Minter into the new one
- Ensuring inflation doesn’t jump up or down due to the migration
- Ensuring users receive their rewards for the round even during the update
Added governor-level integration tests for the end-to-end Minter migration
Updated relevant deployment scripts
Applied small fixes:
- Removed redundant check for the minInflation lower bound
- Updated deploy_contracts.ts logic to match on-chain state
- Updated IMinter.sol interface
- Fixed typos in the comments / test names

How did you test each of these updates (required)

We used the existing hardhat setup to extend the coverage for the newly added code:

New tests for the new constructor arguments
- maxInflation constructor argument can’t be > 100%
- minInflation constructor argument can’t be > 100%
- minInflation constructor argument can’t be > maxInflation
- minInflation constructor argument can be same as maxInflation and can be 0
New tests for the new setter functions
- setMinInflation function can only be called by the Controller owner
- setMinInflation function fails if provided minInflation > 100%
- setMinInflation function fails if provided minInflation > maxInflation
- setMinInflation can set minInflation to 0, maxInflation or other valid value
- setMaxInflation function can only be called by the Controller owner
- setMaxInflation function fails if provided maxInflation > 100%
- setMaxInflation function fails if provided maxInflation < minInflation
- setMaxInflation can set maxInflation to 0, minInflation or other valid value
New tests for the new function to migrate old minter state
- migrateOldMinterState function can only be called by the Controller owner
- migrateOldMinterState function can only be called when old minter is still registered in the controller
- migrateOldMinterState function successfully transfers old minter variables to the current minter
- New tests for the updated inflation logic
- inflation is increased up to maxInflation IF current < targetBondingRate
- inflation is increased up to minInflation IF inflation < minInflation (independent from the bonding rate)
- inflation is decreased down to minInflation IF current > targetBondingRate
- inflation is decreased down to maxInflation IF inflation > maxInflation (independent from the bonding rate)
- inflation is maintained IF current = targetBondingRate AND within minInflation and maxInflation
- inflation is maintained IF minInflation = maxInflation(independent from the bonding rate)
New integration tests
- “Complex update: migrate to new Minter” test is updated to mimic real-world update sequence:
  1. Call migrateToNewMinter on the previous Minter
  2. Call migrateOldMinterState on the new Minter
  3. Grant MINTER_ROLE to the new Minter
  4. Revoke MINTER_ROLE from the previous Minter
  5. Register new Minter in the Controller
- MinterUpgrade that previously ensured that internal state is zeroed, changed to ensure state of the 3 “dynamic” variables is correctly migrated
- MinterUpgrade that previously ensured that user received 0 rewards after the upgrade is changed to ensure received reward is greater than 0

We also took an opportunity to improve the CI by:

Adding an additional test:coverage:check command that exits with non-zero code, if coverage is not 100%
Adding an additional step in the test.yaml workflow that executes the above command

To prepare for the new Minter deployment, we’ve also manually done the following checks:

ℹ️ Checked current production Minter contract (found in the docs) deployed at 0xc20DE37170B45774e6CD3d2304017fc962f27252 on Feb-17-2022
✅ The verified code on Arbiscan matches f5facdf commit
✅ Inspected changes to the contract since the last deployment: only the imported contracts were changed, but none of the changes are critical:
- @openzeppelin/contracts/token/ERC20/IERC20.sol dependency was updated. Here is the diff: only comments and argument names were changed, this has no effect on the actual bytecode
- @openzeppelin/contracts/utils/math/SafeMath.sol dependency was updated. Here is the diff: only comments and formatting of the code were changed, this has no effect on the actual bytecode
- contracts/bonding/IBondingManager.sol was updated. Here is the diff: 1 new event and 2 new functions were added, unused by the Minter contract

Does this pull request close any open issues?

No, but it addresses the discussion started on Livepeer Forum and in the original PR.

Checklist:

~~README and other documentation updated~~
ℹ️ We found no relevant documentation of the Minter contract inside this repository
All tests using yarn test pass

Next steps

Receive reviews from the core contributors
Set actual maxInflation and minInflation values for the deployment (although not a blocker for this PR, since those values can be changed after the deployment during the actual governor update)

SidestreamCrunchyCarrot · 2025-04-01T10:20:31Z

Suggested further actions:

Increase test coverage on the Minter contract to 100% (currently it's 98.28)
~~Increase test coverage on other treasury/* contracts~~ it seems to be excluded from coverage
Enforce coverage threshold to be 100% in the CI (as delta branch already have full coverage)

package.json

deploy/deploy_contracts.ts

deploy/migrations.config.ts

victorges · 2025-04-11T20:39:24Z

contracts/token/Minter.sol

+        // Transfer state from old Minter
+        currentMintableTokens = oldMinter.currentMintableTokens();
+        currentMintedTokens = oldMinter.currentMintedTokens();
+        inflation = oldMinter.inflation();


Should we copy all fields including inflationChange and targetBondingRate to make sure we get a consistent state snaphot? Otherwise, we'd be relying on the transaction that constructed the contract to have initialized it with the same values as the current minter.

We usually run the contract creation separately so that we have a fixed address for the governor transaction. In that case it probably makes sense to make the upgrade tx "self-contained", copying all state, not relying on args configured in other txs. WDYT?

There are two reasons why we thought it would rather be more confusing:

We wouldn't be able to copy all possible fields, since maxInflation and minInflation are not part of the currently deployed contract. In other words it would be confusing during the next upgrade that everything is copied except for those 2 values

The overwrite of other arguments is redundant and can create additional confusion. For example, in case governance decides to also change some of the configurable parameters (e.g. inflationChange) during the Minter upgrade, it will result in 1) deploying the contract with the new or some values 2) calling migrateOldMinterState that will reset them to the old state 3) calling set* functions to set them to the new values

Based on this, we've decided to update only the minimum required state that is dynamically changing.

The problem is that the contracts are not created on the governance transaction, so someone would need to have run a separate transaction first to deploy the new Minter contract. This transaction would not be on governor-scripts repo so it would need to be reviewed separately, creating further complexity on the deployment flow (or at least that's how I remember we did it before).

So IMO an extra transaction to change any other parameters that the governance wants would be preferred, as it would also be very explicit (and not set on a "deploy contract" transaction that is not necessarily audited on the governance process).

Regarding not copying the maxInflation and minInflation, I think it could be covered by a local comment saying that the fields were added with the migration function, so they should be copied if a new version is made. WDYT?

creating further complexity on the deployment flow (or at least that's how I remember we did it before).

I'm not sure where the additional complexity is coming from, as the deployed contract and its state have to be reviewed anyway. Typically, the review of the contract should be part of the standard "governance payload review": if the payload mentions/interacts with a new contract, the code as well as the state have to be checked. In my opinion, having 2 places where a state variable is being modified is rather confusing than "explicit".

So IMO an extra transaction to change any other parameters that the governance wants would be preferred, as it would also be very explicit (and not set on a "deploy contract" transaction that is not necessarily audited on the governance process).

Same problem here: do you then would also suggest to set inflation ceiling and floor in the governance payload to make it explicit? (although those values have already been set at deployment via the constructor arguments)

Regarding not copying the maxInflation and minInflation, I think it could be covered by a local comment

If we still would go with updating migrateOldMinterState scope, what do you mean by the "local" comment? Some kind of TODO comment inside the function (e.g. "TODO: add inflationCeiling and inflationFloor once old miter has those values")?

test/integration/GovernorUpdate.js

test/unit/Minter.js

contracts/token/Minter.sol

codecov · 2025-04-24T23:26:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00000%. Comparing base (e8b6243) to head (78176a0).

Additional details and impacted files

@@               Coverage Diff               @@
##                delta         #645   +/-   ##
===============================================
  Coverage   100.00000%   100.00000%           
===============================================
  Files              29           29           
  Lines            1331         1354   +23     
  Branches          223          232    +9     
===============================================
+ Hits             1331         1354   +23

Files with missing lines	Coverage Δ
contracts/token/Minter.sol	`100.00000% <100.00000%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e8b6243...78176a0. Read the comment docs.

Files with missing lines	Coverage Δ
contracts/token/Minter.sol	`100.00000% <100.00000%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

victorges

LGTM! The only required change is fixing the name of the setinflationCeiling/Floor functions

victorges · 2025-04-24T23:20:13Z

contracts/token/IMinter.sol

+    function inflationCeiling() external view returns (uint256);

-    function minInflation() external view returns (uint256);
+    function inflationFloor() external view returns (uint256);


I like the rename! Make sure there's no occurences of [Mm]ax.+[Ii]nflation on the project anymore 👀

victorges · 2025-04-24T23:23:04Z

contracts/token/Minter.sol

+     * @param _inflationFloor New inflation floor as a percentage of total token supply
     */
-    function setMinInflation(uint256 _minInflation) external onlyControllerOwner {
+    function setinflationFloor(uint256 _inflationFloor) external onlyControllerOwner {


Probably mis-replaced the name here:

Suggested change

function setinflationFloor(uint256 _inflationFloor) external onlyControllerOwner {

function setInflationFloor(uint256 _inflationFloor) external onlyControllerOwner {

Will need to rename in other places like tests etc

victorges · 2025-04-24T23:23:56Z

contracts/token/Minter.sol

+     * @param _inflationCeiling New inflation cap as a percentage of total token supply
     */
-    function setMaxInflation(uint256 _maxInflation) external onlyControllerOwner {
+    function setinflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner {


Same

Suggested change

function setinflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner {

function setInflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner {

victorges · 2025-04-24T23:25:29Z

test/unit/Minter.js

-            const startInflation = await minter.inflation()
+        // Ensure the logic is unaffected by the target bonding rate
+        for (const targetBondingRate of [400, 500, 600]) {
+            it("inflation is increased IF inflation < inflationFloor (independent from bonding rate)", async () => {


nit: Even better than the parenthesis would be specifying the bonding rate on the test title

Suggested change

it("inflation is increased IF inflation < inflationFloor (independent from bonding rate)", async () => {

it($"inflation is increased IF inflation < inflationFloor (targetBondingRate=${targetBondingRate})", async () => {

Same below

stronk-dev and others added 2 commits February 19, 2025 11:27

Add inflation bounds to inflation adjustment algorithm

7201cf0

add tests and a new migrateOldMinterState function

f0f6687

SidestreamSweatyPumpkin added 2 commits April 2, 2025 11:40

100% coverage

06e4647

enforce 100% test coverage in the CI

48e4f36

victorges reviewed Apr 11, 2025

View reviewed changes

SidestreamSweatyPumpkin added 6 commits April 22, 2025 10:33

rename transferStateData->migrateOldMinterState in the tests

73de871

additionally check revert message

161d634

improve tests which are independent from bonding rate

2a602e0

add TODO to set correct inflation bounds

3030b8c

rename maxInflation->inflationCeiling, minInflation->inflationFloor

7f2385e

set correct minter values in the migrations.config and deploy_minter

78176a0

victorges changed the title ~~Finish Minter update~~ minter: Add inflation bounds to inflation adjustment algorithm Apr 24, 2025

victorges mentioned this pull request Apr 24, 2025

Add inflation bounds to inflation adjustment algorithm #644

Closed

2 tasks

victorges previously approved these changes Apr 24, 2025

View reviewed changes

SidestreamSweatyPumpkin added 3 commits April 28, 2025 09:43

fix typo in setInflationCeiling, setInflationFloor

e317d26

improve variable naming consistency in tests after renaming

7dbc7f7

improve test naming

73d0e9b

SidestreamSweatyPumpkin dismissed victorges’s stale review via 73d0e9b April 28, 2025 08:45

Minter deployment artifacts

a1f8096

	function setinflationFloor(uint256 _inflationFloor) external onlyControllerOwner {
	function setInflationFloor(uint256 _inflationFloor) external onlyControllerOwner {

	function setinflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner {
	function setInflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner {

	it("inflation is increased IF inflation < inflationFloor (independent from bonding rate)", async () => {
	it($"inflation is increased IF inflation < inflationFloor (targetBondingRate=${targetBondingRate})", async () => {

minter: Add inflation bounds to inflation adjustment algorithm #645

Are you sure you want to change the base?

minter: Add inflation bounds to inflation adjustment algorithm #645

Uh oh!

Conversation

SidestreamSweatyPumpkin commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SidestreamCrunchyCarrot commented Apr 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

victorges Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

SidestreamSweatyPumpkin Apr 22, 2025

Choose a reason for hiding this comment

Uh oh!

victorges Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SidestreamSweatyPumpkin Apr 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 24, 2025

Codecov Report

Uh oh!

victorges left a comment

Choose a reason for hiding this comment

Uh oh!

victorges Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

victorges Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

victorges Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

victorges Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

SidestreamSweatyPumpkin commented Mar 26, 2025 •

edited

Loading

SidestreamCrunchyCarrot commented Apr 1, 2025 •

edited

Loading

victorges Apr 24, 2025 •

edited

Loading