Skip to content
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.

Sign up for GitHub

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

Jump to bottom

contracts-bedrock: add withdrawal network and make withdrawal minimum configurable #5886

Merged
merged 4 commits into from
Jun 12, 2023
Merged

contracts-bedrock: add withdrawal network and make withdrawal minimum configurable #5886

merged 4 commits into from
Jun 12, 2023

Conversation

dapperscene6
Copy link
Contributor

@dapperscene6 dapperscene6 commented Jun 2, 2023 •
edited
Loading

Description
Modify L1FeeVault, BaseFeeVault, and SequencerFeeVault to have a configurable minimum withdrawal amounts and to be configured to withdraw to L1 or L2.

This changes adds deploy-configuration parameters to already deployed chains which leads to said chains current deploy-configurations no longer being able to recreate the genesis state of said already deployed chains.

Hexadecimals were used for MinimumWithdrawalAmount configuration in deploy-configuration for consistency though @smartcontracts and I discussed changing all deploy-configuration hexadecimals to decimal to increase deploy-configuration readability.

Running make bindings in op-binding created a larger diff than expected which I believe is causing some of the CI tests to fail.

Tests
I've added solidity tests for the smart contract changes and golang tests for the golang changes.

Invariants

TBD

Additional context

These changes are necessary to enable daily revenue share on L2 as well as to reduce withdrawal fees on L1 by aggregating FeeVault earning prior to withdrawing to L1.

Metadata

  • Fixes #[Link to Issue]

TODOs

@dapperscene6 dapperscene6 requested review from a team as code owners June 2, 2023 20:35
@dapperscene6 dapperscene6 requested review from Inphi and clabby June 2, 2023 20:35
@changeset-bot
Copy link

changeset-bot bot commented Jun 2, 2023 •
edited
Loading

⚠️ No Changeset found

Latest commit: 01b9d83

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Jun 2, 2023 •
edited
Loading

✅ Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 01b9d83
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/648699acb23a040008907554

@dapperscene6 dapperscene6 changed the title contracts-bedrock: add withdrawal network and make withdrawal minimum configurable [DRAFT] contracts-bedrock: add withdrawal network and make withdrawal minimum configurable Jun 2, 2023
@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch 6 times, most recently from bd1bd8a to 46006c8 Compare June 8, 2023 17:21
@mergify
Copy link
Contributor

mergify bot commented Jun 9, 2023

Hey @dapperscene6! This PR has merge conflicts. Please fix them before continuing review.

@mergify mergify bot added the conflict label Jun 9, 2023
@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch from 46006c8 to 4f21e80 Compare June 9, 2023 15:43
@mergify mergify bot removed the conflict label Jun 9, 2023
@refcell refcell requested review from refcell and tynes and removed request for clabby and Inphi June 9, 2023 17:18
@github-actions github-actions bot added the C-protocol-critical Category: Modifies protocol-critical code label Jun 9, 2023
@mergify mergify bot requested a review from smartcontracts June 9, 2023 21:17
tynes
tynes reviewed Jun 9, 2023
View reviewed changes
packages/contracts-bedrock/src/deploy-config.ts Outdated
@@ -136,19 +136,49 @@ interface RequiredDeployConfig {
proxyAdminOwner: string

/**
* L1 address which receives the base fee for the L2 network.
* L1 or L2 address which receives the base fee for the L2 network.
Copy link
Contributor

Choose a reason for hiding this comment

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

Local or remote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tynes could you clarify what you're suggesting? Do you mean changing these comments to be: Local or remote address which receives the base fee for the L2 network.?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes exactly, I think using local or remote is better language because it generalizes to L3+. Its def not consistent throughout the codebase

Copy link
Contributor

Choose a reason for hiding this comment

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

If I were to read "Local or remote address", personally I think I'd be very confused without context. I think a clearer and still more flexible expression may be something like

L1 or higher (eg L2) address which receives the base fee for the L2 network.

@tynes
Copy link
Contributor

tynes commented Jun 9, 2023

Not your fault but this is way too large of a diff for a change like this. Its the fault of how the codebase is architected. I'm thinking of ways to reduce friction in the future so that diffs can be smaller with these sorts of changes

@tynes
Copy link
Contributor

tynes commented Jun 9, 2023

I was able to reproduce 7e22976 locally

@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch from fda7c46 to f9b6e46 Compare June 9, 2023 21:47
refcell
refcell requested changes Jun 10, 2023
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

Mostly echoing @tynes comments here, otherwise almost there!

packages/contracts-bedrock/deploy-config/getting-started.json Show resolved Hide resolved
packages/contracts-bedrock/deploy-config/goerli.json Show resolved Hide resolved
packages/contracts-bedrock/src/deploy-config.ts Outdated
@@ -136,19 +136,49 @@ interface RequiredDeployConfig {
proxyAdminOwner: string

/**
* L1 address which receives the base fee for the L2 network.
* L1 or L2 address which receives the base fee for the L2 network.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I were to read "Local or remote address", personally I think I'd be very confused without context. I think a clearer and still more flexible expression may be something like

L1 or higher (eg L2) address which receives the base fee for the L2 network.

@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch from f9b6e46 to e1037ac Compare June 10, 2023 20:42
refcell
refcell requested changes Jun 10, 2023
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

Just two small corrections and I believe we're gtm

packages/contracts-bedrock/deploy-config/getting-started.json Outdated Show resolved Hide resolved
packages/contracts-bedrock/deploy-config/goerli.json Outdated Show resolved Hide resolved
@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch from e1037ac to af94160 Compare June 10, 2023 21:32
@dapperscene6 dapperscene6 force-pushed the dapperscene6/modify-fee-vault branch from 01442aa to 87b2819 Compare June 12, 2023 00:56
@tynes tynes changed the title [DRAFT] contracts-bedrock: add withdrawal network and make withdrawal minimum configurable contracts-bedrock: add withdrawal network and make withdrawal minimum configurable Jun 12, 2023
@tynes
Copy link
Contributor

tynes commented Jun 12, 2023

This looks good to me!

@tynes tynes merged commit 8fe07a2 into ethereum-optimism:develop Jun 12, 2023
kahuang
kahuang reviewed Jul 6, 2023
View reviewed changes
packages/contracts-bedrock/contracts/universal/FeeVault.sol
/**
* @notice Network which the RECIPIENT will receive fees on.
*/
WithdrawalNetwork public immutable WITHDRAWAL_NETWORK;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to upgrade existing fee vault contracts, does this variable need to be moved below the totalProcessed variable, to preserve storage slots?

Copy link
Contributor

Choose a reason for hiding this comment

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

Immutables are inlined in the bytecode, it's not in storage so this doesn't change the storage layout

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kahuang kahuang kahuang left review comments

@tynes tynes tynes approved these changes

@refcell refcell refcell approved these changes

@smartcontracts smartcontracts Awaiting requested review from smartcontracts

Assignees
No one assigned
Labels
C-protocol-critical Category: Modifies protocol-critical code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dapperscene6 @tynes @kahuang @refcell @mslipper