-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add inflation bounds to inflation adjustment algorithm #644
base: delta
Are you sure you want to change the base?
Conversation
4d664a8
to
7201cf0
Compare
@@ -23,6 +23,10 @@ contract Minter is Manager, IMinter { | |||
|
|||
// Per round inflation rate | |||
uint256 public inflation; | |||
// Max per round inflation rate | |||
uint256 public maxInflation; |
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 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.
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.
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)
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.
Took a quick look at this, and while this is not intended to be a complete spec, I see the following complexity in this...
-
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.)
-
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.
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.
- 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",
],
},
];
- 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.
What does this pull request do? Explain your changes. (required)
Modifies the inflation adjustment algorithm to stay withing a set floor and ceiling.
Specific updates (required)
maxInflation
parameterminInflation
parametersetInflation
function to always stay within the floor and ceilingHow did you test each of these updates (required)
TBA
Checklist:
yarn test
pass