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

feat: custom gas token #10143

Merged
merged 45 commits into from
May 2, 2024
Merged

feat: custom gas token #10143

merged 45 commits into from
May 2, 2024

Conversation

tynes
Copy link
Contributor

@tynes tynes commented Apr 12, 2024

Description

Implements the custom gas token spec found in ethereum-optimism/specs#111
Includes the full feature set and extensive test coverage.

  • contracts-bedrock: custom gas token L1 contracts
  • contracts-bedrock: custom gas token L2 contracts
  • contracts-bedrock: custom gas token contracts
  • contracts-bedrock: custom gas token scripts
  • contracts-bedrock: custom gas token tests
  • op-bindings: update with custom gas token
  • op-chain-ops: fixup tests
  • contracts-bedrock: snapshots

Copy link

codecov bot commented Apr 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 29.22%. Comparing base (53f0080) to head (c21302e).
Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #10143       +/-   ##
============================================
- Coverage    42.33%   29.22%   -13.11%     
============================================
  Files           73       31       -42     
  Lines         4845     2898     -1947     
  Branches       766      614      -152     
============================================
- Hits          2051      847     -1204     
+ Misses        2684     1976      -708     
+ Partials       110       75       -35     
Flag Coverage Δ
cannon-go-tests ?
chain-mon-tests 27.14% <ø> (ø)
common-ts-tests ?
contracts-ts-tests 12.25% <ø> (ø)
core-utils-tests ?
sdk-tests 40.27% <ø> (ø)

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

see 42 files with indirect coverage changes

@mds1
Copy link
Contributor

mds1 commented Apr 12, 2024

Looks really great on my first pass. Next week I'd like to re-review the spec and give a more security-focused review of the code and tests in this PR

@tynes
Copy link
Contributor Author

tynes commented Apr 13, 2024

@mds1 Needs versions such that they are -beta+custom-gas-token

@0xfuturistic
Copy link
Contributor

implemented comments here: #10219

@tynes tynes added the M-do-not-merge Meta: Do not merge label Apr 19, 2024
@tynes tynes force-pushed the feat/custom-gas-token branch 2 times, most recently from 015aa3e to 7ec7956 Compare April 23, 2024 03:44
Copy link
Contributor

semgrep-app bot commented Apr 23, 2024

Semgrep found 6 sol-style-return-arg-fmt findings:

Named return arguments to functions must be appended with an underscore (_)

Ignore this finding from sol-style-return-arg-fmt.

@tynes
Copy link
Contributor Author

tynes commented Apr 23, 2024

@mds1 This branch has been rebased on top of your codesize checks and also includes all of the recent specs changes (IGasToken). I think this PR should be good to go at this point

@tynes tynes marked this pull request as ready for review April 23, 2024 23:14
@tynes tynes requested review from a team as code owners April 23, 2024 23:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Out of diff range and nitpick comments (1)
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)

Line range hint 697-697: Avoid using blockhash(block.number) as it always returns 0.

- blockhash(block.number)
+ blockhash(block.number - 1) // Use the hash of the previous block

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Out of diff range and nitpick comments (1)
packages/contracts-bedrock/test/L1/OptimismPortal.t.sol (1)

Line range hint 697-697: Avoid using blockhash(block.number) as it always returns 0.

- optimismPortal.l2Oracle().proposeL2Output(proposal.outputRoot, optimismPortal.l2Oracle().nextBlockNumber(), blockhash(block.number), block.number);
+ optimismPortal.l2Oracle().proposeL2Output(proposal.outputRoot, optimismPortal.l2Oracle().nextBlockNumber(), blockhash(block.number - 1), block.number);

Using blockhash(block.number) results in a value of 0, which is not useful. Use blockhash(block.number - 1) instead to get the hash of the most recent block.

Copy link
Contributor

@trianglesphere trianglesphere left a comment

Choose a reason for hiding this comment

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

go changes LGTM

@tynes tynes added this pull request to the merge queue May 2, 2024
Merged via the queue into develop with commit a6074a7 May 2, 2024
72 of 74 checks passed
@tynes tynes deleted the feat/custom-gas-token branch May 2, 2024 20:51
mds1 added a commit that referenced this pull request May 2, 2024
tynes added a commit that referenced this pull request May 2, 2024
github-merge-queue bot pushed a commit that referenced this pull request May 2, 2024
@tynes tynes restored the feat/custom-gas-token branch May 2, 2024 22:17
tynes added a commit that referenced this pull request May 3, 2024
Now that the fault proof contracts release has happened, we
can merge in the custom gas token changes.

This reverts commit 2b1c99b.
github-merge-queue bot pushed a commit that referenced this pull request May 3, 2024
Now that the fault proof contracts release has happened, we
can merge in the custom gas token changes.

This reverts commit 2b1c99b.
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