Allow an authorized address to modify the parameters which determine the gas burn#1516
Conversation
🦋 Changeset detectedLatest commit: d9ebd84 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Codecov Report
@@ Coverage Diff @@
## experimental #1516 +/- ##
================================================
- Coverage 74.20% 73.62% -0.58%
================================================
Files 67 67
Lines 2210 2207 -3
Branches 321 324 +3
================================================
- Hits 1640 1625 -15
- Misses 570 582 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Now that I think about it, it's probably not the sequencer we want here.
Do we need a new entry in the AddressManager?
There was a problem hiding this comment.
Yes, we should have a ConfigManagerOwner or something like that
There was a problem hiding this comment.
What is that the name of? Would it be a function that should be added to the CTC or AM?
How would the code above change for that, like this?
| msg.sender == resolve("OVM_Sequencer"), | |
| modifier onlyManager() { | |
| require( | |
| msg.sender == resolve("OVM_Manager"), |
There was a problem hiding this comment.
Should we just set this to the owner of the AddressManager itself?
I'm concerned about introducing yet another key to our deployments (considering how hard it is already to keep track of everything)
There was a problem hiding this comment.
Going to change this to:
modifier onlyBurnAdmin() {
require(
msg.sender == resolve("OVM_BurnAdmin"),
"Only callable by the BurnAdmin."
);
_;
}
But won't be able to make the needed changes to the deploy scripts for a few hours.
There was a problem hiding this comment.
FWIW I would +1 Karl's suggestion to just use addressManager.owner.
There was a problem hiding this comment.
Maybe it's an argument for using the musig less for this non-security-critical param?
There was a problem hiding this comment.
There's nothing preventing resolve("OVM_BurnAdmin") from returning addressManager.owner or any other address we choose.
So I like this approach as it provides the option to use a unique address or reuse another one.
packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts
Outdated
Show resolved
Hide resolved
eb5a20c to
0a7aee7
Compare
karlfloersch
left a comment
There was a problem hiding this comment.
This LGTM other than the OVM_Sequencer bit - I'd probably use the owner of the address manager.
There was a problem hiding this comment.
Should we just set this to the owner of the AddressManager itself?
I'm concerned about introducing yet another key to our deployments (considering how hard it is already to keep track of everything)
packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
Outdated
Show resolved
Hide resolved
ben-chain
left a comment
There was a problem hiding this comment.
LGTM pending what we want to do with the config!
There was a problem hiding this comment.
FWIW I would +1 Karl's suggestion to just use addressManager.owner.
There was a problem hiding this comment.
Maybe it's an argument for using the musig less for this non-security-critical param?
2231af3 to
406945c
Compare
2f7bf37 to
db53e49
Compare
packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
Outdated
Show resolved
Hide resolved
packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
Outdated
Show resolved
Hide resolved
karlfloersch
left a comment
There was a problem hiding this comment.
Found one tiny change but otherwise LGTM!
packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
Outdated
Show resolved
Hide resolved
packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol
Outdated
Show resolved
Hide resolved
packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts
Outdated
Show resolved
Hide resolved
dd3b123 to
d9ebd84
Compare
Co-authored-by: refcell <abigger87@gmail.com>
Description
Allows us to modify the gas burn threshold, but adding two setter functions, which only the sequencer can call.