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.

Sign up for GitHub

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

Add inflation bounds to inflation adjustment algorithm #644

Draft
wants to merge 1 commit into
base: delta
Choose a base branch
from
Draft

Add inflation bounds to inflation adjustment algorithm #644

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 62 additions & 6 deletions contracts/token/Minter.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ contract Minter is Manager, IMinter {

// Per round inflation rate
uint256 public inflation;
// Max per round inflation rate
uint256 public maxInflation;
Copy link
Member

Choose a reason for hiding this comment

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

I know this is just a draft PR, and will ultimately need thorough review and tests, but just commenting for reference on one issue...

In the past I know it was true that when introducing new storage variables, they actually HAD to be included AFTER existing storage variables in the contract code. Because when the contract was upgraded, the storage layout onchain stayed the same and would load the existing values into the right variables. I do not know if the latest compilers have been updated to solve for this or not. But probably best to include the new variables after all other storage variables have been declared.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the Minter is not an upgradeable proxy like the other contracts so in order to make this change onchain would have to deploy a completely new Minter and register it to replace the current one. (Which means the storage layout stuff wouldn't apply since its a fresh contract)

Copy link
Member

Choose a reason for hiding this comment

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

Took a quick look at this, and while this is not intended to be a complete spec, I see the following complexity in this...

  1. The Minter state would need to be duplicated to the new Minter via either the constructor, or by calling the appropriate setters. So either the deployment would need to be tightly coordinated such that the state values are set to the old Minter's state values exactly, or would need to be scripted to read the old values and feed them as inputs to the constructor. (Requires more testing of the deployment script.)

  2. The function that transfers to a new minter is pretty high stakes. It transfers all ETH and LPT deposited within the protocol from the old Minter to the new Minter. And of course the ownership of these contracts are verified such that it can't accidentally pass the value to an unowned contract. While all testable and scriptable, I'd say the bar is higher for the community gaining confidence in this potential upgrade, than it would be to simply upgrade a target contract in the proxy mechanism.

Commenting to memorialize the potential complexity of this upgrade path versus other options. But LMK if you think I'm misunderstanding anything here.

Copy link
Member

@yondonfu yondonfu Feb 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. Yes the relevant state would need to be duplicated. The steps would roughly be a) deploying the new Minter with the correct parameters and b) using migrateToNewMinter() to move funds from the old Minter to the new Minter and c) adjusting role permissions on the LPT contract d) registering the new Minter with the Controller.

In practice, all of these steps after deploying the new Minter would be batched into a single atomic tx via the Governor - the JSON below roughly captures the steps in the tx (this was for a Minter related upgrade in the past):

module.exports = [
  {
    target: ADDRESSES.arbitrumMainnet.minter,
    value: "0",
    contract: "Minter",
    name: "migrateToNewMinter",
    params: [ADDRESSES.arbitrumMainnet.minterV2],
  },
  {
    target: ADDRESSES.arbitrumMainnet.livepeerToken,
    value: "0",
    contract: "LivepeerToken",
    name: "grantRole",
    params: [
      // keccak256("MINTER_ROLE")
      "0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6",
      ADDRESSES.arbitrumMainnet.minterV2,
    ],
  },
  {
    target: ADDRESSES.arbitrumMainnet.livepeerToken,
    value: "0",
    contract: "LivepeerToken",
    name: "revokeRole",
    params: [
      // keccak256("MINTER_ROLE")
      "0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6",
      ADDRESSES.arbitrumMainnet.minter,
    ],
  },
  {
    target: ADDRESSES.arbitrumMainnet.controller,
    value: "0",
    contract: "Controller",
    name: "setContractInfo",
    params: [
      // keccak256("Minter")
      "0x6e58ad548d72b425ea94c15f453bf26caddb061d82b2551db7fdd3cefe0e9940",
      ADDRESSES.arbitrumMainnet.minterV2,
      "0xbc2078db3096523dac0e663415dda48804fcde4d",
    ],
  },
];
  1. Agreed that this is more complex than a proxy contract upgrade and would require greater scrutiny even if the steps can be batched into a single atomic tx.

// Min per round inflation rate
uint256 public minInflation;
// Change in inflation rate per round until the target bonding rate is achieved
uint256 public inflationChange;
// Target bonding rate
Expand Down Expand Up @@ -70,15 +74,26 @@ contract Minter is Manager, IMinter {
* @param _inflation Base inflation rate as a percentage of current total token supply
* @param _inflationChange Change in inflation rate each round (increase or decrease) if target bonding rate is not achieved
* @param _targetBondingRate Target bonding rate as a percentage of total bonded tokens / total token supply
* @param _maxInflation Inflation rate ceiling as a percentage of current total token supply
* @param _minInflation Inflation rate floor as a percentage of current total token supply
*/
constructor(
address _controller,
uint256 _inflation,
uint256 _inflationChange,
uint256 _targetBondingRate
uint256 _targetBondingRate,
uint256 _maxInflation,
uint256 _minInflation
) Manager(_controller) {
// Inflation must be valid percentage
require(MathUtils.validPerc(_inflation), "_inflation is invalid percentage");
// Inflation bounds must be valid percentages
require(MathUtils.validPerc(_maxInflation), "_maxInflation is invalid percentage");
require(MathUtils.validPerc(_minInflation), "_minInflation is invalid percentage");
// Inflation floor should be >= 0
require(_minInflation >= 0, "_minInflation must be >= 0");
// Inflation floor should be lower or equal to the ceiling
require(_minInflation <= _maxInflation, "_minInflation must be <= _maxInflation");
// Inflation change must be valid percentage
require(MathUtils.validPerc(_inflationChange), "_inflationChange is invalid percentage");
// Target bonding rate must be valid percentage
Expand All @@ -87,6 +102,8 @@ contract Minter is Manager, IMinter {
inflation = _inflation;
inflationChange = _inflationChange;
targetBondingRate = _targetBondingRate;
maxInflation = _maxInflation;
minInflation = _minInflation;
}

/**
Expand Down Expand Up @@ -115,6 +132,38 @@ contract Minter is Manager, IMinter {
emit ParameterUpdate("inflationChange");
}

/**
* @notice Set maxInflation. Only callable by Controller owner
* @param _maxInflation New inflation cap as a percentage of total token supply
*/
function setMaxInflation(uint256 _maxInflation) external onlyControllerOwner {
// Must be valid percentage
require(MathUtils.validPerc(_maxInflation), "_maxInflation is invalid percentage");
// Inflation ceiling should be higher or equal to the floor
require(_maxInflation >= minInflation, "_maxInflation must be >= minInflation");

maxInflation = _maxInflation;

emit ParameterUpdate("maxInflation");
}

/**
* @notice Set minInflation. Only callable by Controller owner
* @param _minInflation New inflation floor as a percentage of total token supply
*/
function setMinInflation(uint256 _minInflation) external onlyControllerOwner {
// Must be valid percentage
require(MathUtils.validPerc(_minInflation), "_minInflation is invalid percentage");
// Inflation floor should be >= 0
require(_minInflation >= 0, "_minInflation must be >= 0");
// Inflation floor should be lower or equal to the ceiling
require(_minInflation <= maxInflation, "_minInflation must be <= maxInflation");

minInflation = _minInflation;

emit ParameterUpdate("minInflation");
}

/**
* @notice Migrate to a new Minter by transferring the current Minter's LPT + ETH balance to the new Minter
* @dev Only callable by Controller owner
Expand Down Expand Up @@ -240,13 +289,20 @@ contract Minter is Manager, IMinter {
currentBondingRate = MathUtils.percPoints(totalBonded, totalSupply);
}

if (currentBondingRate < targetBondingRate) {
// Adjust inflation based on current bonding rate and target bonding rate, ensuring it stays within the floor and ceiling
if ((currentBondingRate < targetBondingRate && inflation < maxInflation) || inflation < minInflation) {
// Bonding rate is below the target - increase inflation
inflation = inflation.add(inflationChange);
} else if (currentBondingRate > targetBondingRate) {
if (inflation.add(inflationChange) > maxInflation) {
// If inflation would go above the ceiling, set it to the ceiling
inflation = maxInflation;
} else {
inflation = inflation.add(inflationChange);
}
} else if ((currentBondingRate > targetBondingRate && inflation > minInflation) || inflation > maxInflation) {
// Bonding rate is above the target - decrease inflation
if (inflationChange > inflation) {
inflation = 0;
if (minInflation.add(inflationChange) > inflation) {
// If inflation would go below the floor, set it to the floor
inflation = minInflation;
} else {
inflation = inflation.sub(inflationChange);
}
Expand Down