-
Notifications
You must be signed in to change notification settings - Fork 46
minter: Add inflation bounds to inflation adjustment algorithm #645
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
base: delta
Are you sure you want to change the base?
Changes from 10 commits
7201cf0
f0f6687
06e4647
48e4f36
73de871
161d634
2a602e0
3030b8c
7f2385e
78176a0
e317d26
7dbc7f7
73d0e9b
a1f8096
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -23,6 +23,10 @@ contract Minter is Manager, IMinter { | |||||
|
|
||||||
| // Per round inflation rate | ||||||
| uint256 public inflation; | ||||||
| // Target maximum inflation rate | ||||||
| uint256 public inflationCeiling; | ||||||
| // Target minimum inflation rate | ||||||
| uint256 public inflationFloor; | ||||||
| // Change in inflation rate per round until the target bonding rate is achieved | ||||||
| uint256 public inflationChange; | ||||||
| // Target bonding rate | ||||||
|
|
@@ -70,15 +74,24 @@ 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 _inflationCeiling Inflation rate ceiling as a percentage of current total token supply | ||||||
| * @param _inflationFloor Inflation rate floor as a percentage of current total token supply | ||||||
| */ | ||||||
| constructor( | ||||||
| address _controller, | ||||||
| uint256 _inflation, | ||||||
| uint256 _inflationChange, | ||||||
| uint256 _targetBondingRate | ||||||
| uint256 _targetBondingRate, | ||||||
| uint256 _inflationCeiling, | ||||||
| uint256 _inflationFloor | ||||||
| ) Manager(_controller) { | ||||||
| // Inflation must be valid percentage | ||||||
| require(MathUtils.validPerc(_inflation), "_inflation is invalid percentage"); | ||||||
| // Inflation bounds must be valid percentages | ||||||
| require(MathUtils.validPerc(_inflationCeiling), "_inflationCeiling is invalid percentage"); | ||||||
| require(MathUtils.validPerc(_inflationFloor), "_inflationFloor is invalid percentage"); | ||||||
| // Inflation floor should be lower or equal to the ceiling | ||||||
| require(_inflationFloor <= _inflationCeiling, "_inflationFloor must be <= _inflationCeiling"); | ||||||
| // Inflation change must be valid percentage | ||||||
| require(MathUtils.validPerc(_inflationChange), "_inflationChange is invalid percentage"); | ||||||
| // Target bonding rate must be valid percentage | ||||||
|
|
@@ -87,6 +100,8 @@ contract Minter is Manager, IMinter { | |||||
| inflation = _inflation; | ||||||
| inflationChange = _inflationChange; | ||||||
| targetBondingRate = _targetBondingRate; | ||||||
| inflationCeiling = _inflationCeiling; | ||||||
| inflationFloor = _inflationFloor; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -115,6 +130,36 @@ contract Minter is Manager, IMinter { | |||||
| emit ParameterUpdate("inflationChange"); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @notice Set inflationCeiling. Only callable by Controller owner | ||||||
| * @param _inflationCeiling New inflation cap as a percentage of total token supply | ||||||
| */ | ||||||
| function setinflationCeiling(uint256 _inflationCeiling) external onlyControllerOwner { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same
Suggested change
|
||||||
| // Must be valid percentage | ||||||
| require(MathUtils.validPerc(_inflationCeiling), "_inflationCeiling is invalid percentage"); | ||||||
| // Inflation ceiling should be higher or equal to the floor | ||||||
| require(_inflationCeiling >= inflationFloor, "_inflationCeiling must be >= inflationFloor"); | ||||||
|
|
||||||
| inflationCeiling = _inflationCeiling; | ||||||
|
|
||||||
| emit ParameterUpdate("inflationCeiling"); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @notice Set inflationFloor. Only callable by Controller owner | ||||||
| * @param _inflationFloor New inflation floor as a percentage of total token supply | ||||||
| */ | ||||||
| function setinflationFloor(uint256 _inflationFloor) external onlyControllerOwner { | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably mis-replaced the name here:
Suggested change
Will need to rename in other places like tests etc |
||||||
| // Must be valid percentage | ||||||
| require(MathUtils.validPerc(_inflationFloor), "_inflationFloor is invalid percentage"); | ||||||
| // Inflation floor should be lower or equal to the ceiling | ||||||
| require(_inflationFloor <= inflationCeiling, "_inflationFloor must be <= inflationCeiling"); | ||||||
|
|
||||||
| inflationFloor = _inflationFloor; | ||||||
|
|
||||||
| emit ParameterUpdate("inflationFloor"); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @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 | ||||||
|
|
@@ -138,6 +183,21 @@ contract Minter is Manager, IMinter { | |||||
| _newMinter.depositETH{ value: address(this).balance }(); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @notice Migrate state variables affected by RoundsManager from the old Minter | ||||||
| * @dev Only callable by Controller owner | ||||||
| */ | ||||||
| function migrateOldMinterState() external onlyControllerOwner { | ||||||
| IMinter oldMinter = IMinter(controller.getContract(keccak256("Minter"))); | ||||||
| // Old Minter cannot be the current Minter | ||||||
| require(address(oldMinter) != address(this), "old Minter cannot be current Minter"); | ||||||
|
|
||||||
| // Transfer state from old Minter | ||||||
| currentMintableTokens = oldMinter.currentMintableTokens(); | ||||||
| currentMintedTokens = oldMinter.currentMintedTokens(); | ||||||
| inflation = oldMinter.inflation(); | ||||||
|
Comment on lines
+195
to
+198
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we copy all fields including 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?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two reasons why we thought it would rather be more confusing:
Based on this, we've decided to update only the minimum required state that is dynamically changing.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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".
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)
If we still would go with updating |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * @notice Create reward based on a fractional portion of the mintable tokens for the current round | ||||||
| * @param _fracNum Numerator of fraction (active transcoder's stake) | ||||||
|
|
@@ -240,13 +300,22 @@ 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 < inflationCeiling) || inflation < inflationFloor) { | ||||||
| // Bonding rate is below the target - increase inflation | ||||||
| inflation = inflation.add(inflationChange); | ||||||
| } else if (currentBondingRate > targetBondingRate) { | ||||||
| if (inflation.add(inflationChange) > inflationCeiling) { | ||||||
| // If inflation would go above the ceiling, set it to the ceiling | ||||||
| inflation = inflationCeiling; | ||||||
| } else { | ||||||
| inflation = inflation.add(inflationChange); | ||||||
| } | ||||||
| } else if ( | ||||||
| (currentBondingRate > targetBondingRate && inflation > inflationFloor) || inflation > inflationCeiling | ||||||
| ) { | ||||||
| // Bonding rate is above the target - decrease inflation | ||||||
| if (inflationChange > inflation) { | ||||||
| inflation = 0; | ||||||
| if (inflationFloor.add(inflationChange) > inflation) { | ||||||
| // If inflation would go below the floor, set it to the floor | ||||||
| inflation = inflationFloor; | ||||||
| } else { | ||||||
| inflation = inflation.sub(inflationChange); | ||||||
| } | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the rename! Make sure there's no occurences of
[Mm]ax.+[Ii]nflationon the project anymore 👀