-
Notifications
You must be signed in to change notification settings - Fork 3.9k
contracts-bedrock: port custom gas token to portal2 #10780
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #10780 +/- ##
============================================
- Coverage 60.69% 16.44% -44.25%
============================================
Files 20 8 -12
Lines 1781 535 -1246
Branches 71 71
============================================
- Hits 1081 88 -993
+ Misses 667 447 -220
+ Partials 33 0 -33
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
@coderabbitai summary |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai Double check that all of the tests from |
This comment was marked as resolved.
This comment was marked as resolved.
2e6b583 to
0f28a73
Compare
|
@mds1 - I went through and made sure all of the tests were ported from the original implementation |
mds1
left a comment
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.
This looks great, seems like the merge was pretty straightforward as expected.
I assume in a follow up PRs we will delete the original portal then rename portal2 to portal?
I went through and made sure all of the tests were ported from the original implementation
Unfortunately I did not get a chance to double check this, but would just ask if we can get someone else to verify none are missing before merge. Worst case I can confirm once I'm back from vacation
Yup, exactly!
Nobody has taken the time to do this review yet, so this is still pending. No major rush, this is nice to have to enable the interop devnet to run with the fault proof code, will rebase #11051 on top of this after its merged on |
|
@mds1 There has been no progress on this PR while you have been on vacation, but I did go through myself to double check that all the tests were ported and didn't find anything missing |
Ports the custom gas token feature to `OptimismPortal2`. This will enable fault proofs to run on custom gas token chains.
|
Semgrep found 2
Please create a GitHub ticket for this TODO. Ignore this finding from todos_require_linear. |
|
@mds1 bump on this, I know you are busy but I do think this is good to merge. It will unblock me from doing another custom gas token release |
mds1
left a comment
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.
There has been no progress on this PR while you have been on vacation, but I did go through myself to double check that all the tests were ported and didn't find anything missing
Thanks, I also confirmed with a regex search of \bfunction\s+test.*Gas.*\b to find all gas related tests in OptimismPortal.t.sol and confirm it matches the set in OptimismPortal2.t.sol, which it did
Since no other changes have been made since my last review, this LGTM. Nice work, it's great to have these merged
Description
Ports the custom gas token feature to
OptimismPortal2.This will enable fault proofs to run on custom gas token
chains.