-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
feat: custom gas token #10143
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
Flags with carried forward coverage won't be shown. Click here to find out more. |
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 |
@mds1 Needs versions such that they are |
5c6a6c1
to
095375f
Compare
implemented comments here: #10219 |
015aa3e
to
7ec7956
Compare
Semgrep found 6
Named return arguments to functions must be appended with an underscore ( |
@mds1 This branch has been rebased on top of your codesize checks and also includes all of the recent specs changes ( |
02085fe
to
50b6f58
Compare
Custom gas token semantics are strengthened and cleaned up to ensure invariants in spec are held true.
c9c91eb
to
e532b85
Compare
There was a problem hiding this 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 usingblockhash(block.number)
as it always returns 0.- blockhash(block.number) + blockhash(block.number - 1) // Use the hash of the previous block
There was a problem hiding this 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 usingblockhash(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. Useblockhash(block.number - 1)
instead to get the hash of the most recent block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go changes LGTM
Description
Implements the custom gas token spec found in ethereum-optimism/specs#111
Includes the full feature set and extensive test coverage.