-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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: ban deposits client tests #11504
feat: ban deposits client tests #11504
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/ban-deposits-interop #11504 +/- ##
=============================================================
- Coverage 78.10% 77.98% -0.12%
=============================================================
Files 28 29 +1
Lines 2137 2167 +30
=============================================================
+ Hits 1669 1690 +21
- Misses 407 413 +6
- Partials 61 64 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
57aa886
to
09411d3
Compare
…ism/optimism into feat/ban-deposits-tests
// cast keccak $(cast concat-hex 0x0000000000000000000000000000000000000000000000000000000000000003 $(cast keccak 0x01)) | ||
// # 0x8afb1c4a581d0e71ab65334e3365ba5511fb15c13fa212776f9d4dafc6287845 | ||
func TestAfterForceIncludeSource(t *testing.T) { | ||
source := AfterForceIncludeSource{ |
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.
nit: this name is not consistent, its after force include, which iirc we moved away from
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.
I think we agreed to go with AfterForceInclude
, but I'm happy to re-name it to a better name :)
// sets config to post-interop | ||
cfg.RegolithTime = &zero64 | ||
cfg.EcotoneTime = &zero64 | ||
cfg.InteropTime = &zero64 | ||
cfg.BlockTime = 2 |
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.
Bedrock, Canyon, Fjord, Granite are missing
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.
Generally looks good. I think it would be nice to have a test helper-function to activate all hardforks in a rollup.Config
up to and including a specific hardfork name. Then it's easier to prepare test cases for specific forks.
ccf505d
into
feat/ban-deposits-interop
Description
this PR is an extension of: #11362 please refer to it for additional context