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.

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

contracts-bedrock: modularize deposit resource config #5233

Merged
merged 30 commits into from
Mar 24, 2023

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Mar 22, 2023

Description

Modularize the deposit resource config so that the source of truth is the SystemConfig contract. Now when users do a deposit, the OptimismPortal will make a call to the SystemConfig to get the params instead of reading them from constant variables. This will not impact the amount of gas users spend when depositing because it tracks the gas usage of the computations required to determine how much gas will be burnt and will burn the difference between the gas used to compute how much gas needs to be burnt and the actual amount of gas to be burnt.

A future PR may have the SystemConfig emit an event when the resource config is updated, this would allow the op-node to track the config for the deposit gas market pricing. Right now it would be a no-op, but in the future we could move the computations that currently happen on L1 to make the 1559 curve into the op-node so that the expensive calculations happen offchain and then allow the user to pay for the gas on L2.

Closes CLI-3704

TODOs

@changeset-bot
Copy link

changeset-bot bot commented Mar 22, 2023

⚠️ No Changeset found

Latest commit: 0daba1e

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 Mar 22, 2023

Deploy Preview for opstack-docs canceled.

Name Link
🔨 Latest commit 0daba1e
🔍 Latest deploy log https://app.netlify.com/sites/opstack-docs/deploys/641dbce616b6710007463d2e

@semgrep-app
Copy link
Contributor

semgrep-app bot commented Mar 23, 2023

Semgrep found 2 unchecked-type-assertion findings:

  • op-bindings/bindings/systemconfig.go: L504
  • op-bindings/bindings/optimismportal.go: L388

Unchecked type assertion.

Created by unchecked-type-assertion.

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.

Nice; lovingly grated, tastefully made

@tynes tynes marked this pull request as ready for review March 23, 2023 20:58
@tynes tynes requested review from a team as code owners March 23, 2023 20:58
@tynes tynes force-pushed the fix/syscfg-prevent-low-gaslimit-3 branch from a28336f to 0ed8dca Compare March 23, 2023 21:06
@smartcontracts
Copy link
Contributor

Marking do-not-merge while review happens

@tynes
Copy link
Contributor Author

tynes commented Mar 23, 2023

seems like the majority of comments are around documentation

Copy link
Contributor

@maurelian maurelian left a comment

Choose a reason for hiding this comment

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

Approving on the basis that merging will proceed once comments have been resolved.

Copy link
Member

@clabby clabby left a comment

Choose a reason for hiding this comment

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

LGTM once CI is green

@maurelian
Copy link
Contributor

removed do-not-merge. No comments were blocking. We're just working through some common CI issues.

@tynes tynes merged commit 9b9f78c into develop Mar 24, 2023
@tynes tynes deleted the fix/syscfg-prevent-low-gaslimit-3 branch March 24, 2023 15:30
tynes added a commit that referenced this pull request Mar 24, 2023
The gas limit must be set to 30 million to ensure that
the default deposit gas market config is safe to use.
This was missed when
#5233 was merged.
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.

5 participants