-
Notifications
You must be signed in to change notification settings - Fork 47
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
Open
SidestreamSweatyPumpkin
wants to merge
14
commits into
livepeer:delta
Choose a base branch
from
sidestream-tech:finish-minter-update
base: delta
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
7201cf0
Add inflation bounds to inflation adjustment algorithm
stronk-dev f0f6687
add tests and a new migrateOldMinterState function
SidestreamSweatyPumpkin 06e4647
100% coverage
SidestreamSweatyPumpkin 48e4f36
enforce 100% test coverage in the CI
SidestreamSweatyPumpkin 73de871
rename transferStateData->migrateOldMinterState in the tests
SidestreamSweatyPumpkin 161d634
additionally check revert message
SidestreamSweatyPumpkin 2a602e0
improve tests which are independent from bonding rate
SidestreamSweatyPumpkin 3030b8c
add TODO to set correct inflation bounds
SidestreamSweatyPumpkin 7f2385e
rename maxInflation->inflationCeiling, minInflation->inflationFloor
SidestreamSweatyPumpkin 78176a0
set correct minter values in the migrations.config and deploy_minter
SidestreamSweatyPumpkin e317d26
fix typo in setInflationCeiling, setInflationFloor
SidestreamSweatyPumpkin 7dbc7f7
improve variable naming consistency in tests after renaming
SidestreamSweatyPumpkin 73d0e9b
improve test naming
SidestreamSweatyPumpkin a1f8096
Minter deployment artifacts
SidestreamSweatyPumpkin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Should we copy all fields including
inflationChange
andtargetBondingRate
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 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.
There are two reasons why we thought it would rather be more confusing:
maxInflation
andminInflation
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 valuesinflationChange
) during the Minter upgrade, it will result in 1) deploying the contract with the new or some values 2) callingmigrateOldMinterState
that will reset them to the old state 3) callingset*
functions to set them to the new valuesBased on this, we've decided to update only the minimum required state that is dynamically changing.
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.
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 ongovernor-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
andminInflation
, 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?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'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
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")?