Skip to content

Allow an authorized address to modify the parameters which determine the gas burn#1516

Merged
maurelian merged 6 commits intoexperimentalfrom
maurelian/eng-1470-determine-any-next-steps-due-to
Oct 7, 2021
Merged

Allow an authorized address to modify the parameters which determine the gas burn#1516
maurelian merged 6 commits intoexperimentalfrom
maurelian/eng-1470-determine-any-next-steps-due-to

Conversation

@maurelian
Copy link
Copy Markdown
Contributor

Description

Allows us to modify the gas burn threshold, but adding two setter functions, which only the sequencer can call.

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Oct 2, 2021

🦋 Changeset detected

Latest commit: d9ebd84

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@eth-optimism/contracts Minor
@eth-optimism/batch-submitter Patch
@eth-optimism/data-transport-layer Patch
@eth-optimism/message-relayer Patch
@eth-optimism/regenesis-surgery Patch

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

@github-actions github-actions bot added 2-reviewers A-op-batcher Area: op-batcher M-ci Meta: ci related work A-pkg-core-utils Area: packages/core-utils A-integration Area: integration tests A-cannon Area: cannon A-ops Area: ops labels Oct 2, 2021
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Oct 2, 2021

Codecov Report

Merging #1516 (d9ebd84) into experimental (b690575) will decrease coverage by 0.57%.
The diff coverage is 75.00%.

❗ Current head d9ebd84 differs from pull request most recent head dd3b123. Consider uploading reports for the commit dd3b123 to get more accurate results
Impacted file tree graph

@@               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     
Flag Coverage Δ
batch-submitter 61.74% <ø> (ø)
contracts 90.44% <90.47%> (-0.74%) ⬇️
core-utils 55.73% <28.57%> (-0.77%) ⬇️
data-transport-layer 38.23% <ø> (ø)
message-relayer 83.17% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core-utils/src/common/hex-strings.ts 86.11% <28.57%> (-13.89%) ⬇️
.../contracts/L1/rollup/CanonicalTransactionChain.sol 85.04% <90.47%> (-2.89%) ⬇️
...acts/contracts/L1/rollup/ChainStorageContainer.sol 70.00% <0.00%> (-10.00%) ⬇️
...contracts/contracts/libraries/utils/Lib_Buffer.sol 92.85% <0.00%> (-7.15%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a787fc3...dd3b123. Read the comment docs.

@maurelian maurelian changed the base branch from develop to experimental October 2, 2021 01:21
@github-actions github-actions bot removed A-cannon Area: cannon A-pkg-core-utils Area: packages/core-utils A-integration Area: integration tests M-dtl A-ops Area: ops A-op-batcher Area: op-batcher M-ci Meta: ci related work labels Oct 2, 2021
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Now that I think about it, it's probably not the sequencer we want here.
Do we need a new entry in the AddressManager?

Copy link
Copy Markdown
Contributor

@smartcontracts smartcontracts Oct 4, 2021

Choose a reason for hiding this comment

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

Yes, we should have a ConfigManagerOwner or something like that

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Suggested change
msg.sender == resolve("OVM_Sequencer"),
modifier onlyManager() {
require(
msg.sender == resolve("OVM_Manager"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FWIW I would +1 Karl's suggestion to just use addressManager.owner.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it's an argument for using the musig less for this non-security-critical param?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@maurelian maurelian force-pushed the maurelian/eng-1470-determine-any-next-steps-due-to branch from eb5a20c to 0a7aee7 Compare October 2, 2021 15:07
@maurelian maurelian requested review from ben-chain and tynes October 2, 2021 16:06
Copy link
Copy Markdown
Contributor

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

This LGTM other than the OVM_Sequencer bit - I'd probably use the owner of the address manager.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

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

LGTM pending what we want to do with the config!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FWIW I would +1 Karl's suggestion to just use addressManager.owner.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe it's an argument for using the musig less for this non-security-critical param?

@github-actions github-actions bot added the A-ops Area: ops label Oct 5, 2021
@maurelian maurelian force-pushed the maurelian/eng-1470-determine-any-next-steps-due-to branch 2 times, most recently from 2231af3 to 406945c Compare October 5, 2021 15:10
@maurelian maurelian changed the title Allow the sequencer to modify the parameters which determine the gas burn Allow an authorized address to modify the parameters which determine the gas burn Oct 5, 2021
@maurelian maurelian requested a review from karlfloersch October 5, 2021 15:29
@maurelian maurelian force-pushed the maurelian/eng-1470-determine-any-next-steps-due-to branch from 2f7bf37 to db53e49 Compare October 6, 2021 02:10
@github-actions github-actions bot removed the A-ops Area: ops label Oct 6, 2021
@maurelian maurelian removed the request for review from tynes October 6, 2021 14:04
Copy link
Copy Markdown
Contributor

@karlfloersch karlfloersch left a comment

Choose a reason for hiding this comment

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

Found one tiny change but otherwise LGTM!

@maurelian maurelian force-pushed the maurelian/eng-1470-determine-any-next-steps-due-to branch from dd3b123 to d9ebd84 Compare October 6, 2021 23:38
@maurelian maurelian merged commit 4c7a82e into experimental Oct 7, 2021
@maurelian maurelian deleted the maurelian/eng-1470-determine-any-next-steps-due-to branch October 7, 2021 00:07
@github-actions github-actions bot mentioned this pull request Nov 10, 2021
theochap added a commit that referenced this pull request Dec 10, 2025
Co-authored-by: refcell <abigger87@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants