-
Notifications
You must be signed in to change notification settings - Fork 22
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
refactor(blockifier): refactor default testing resource bounds #1306
refactor(blockifier): refactor default testing resource bounds #1306
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @dorimedini-starkware and the rest of your teammates on Graphite |
5182295
to
3098d59
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1306 +/- ##
==========================================
- Coverage 74.18% 71.94% -2.25%
==========================================
Files 359 357 -2
Lines 36240 37960 +1720
Branches 36240 37960 +1720
==========================================
+ Hits 26886 27310 +424
- Misses 7220 8534 +1314
+ Partials 2134 2116 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
9d5d04e
to
b7510f2
Compare
404d032
to
42b94c9
Compare
Benchmark movements: |
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.
Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware)
crates/blockifier/src/test_utils.rs
line 100 at r1 (raw file):
// The max_fee / resource bounds used for txs in this test. // V3 transactions: pub const DEFAULT_L1_GAS_AMOUNT: GasAmount = GasAmount(1000000);
Suggestion:
u64::pow(10, 6)
crates/blockifier/src/test_utils.rs
line 120 at r1 (raw file):
// The amount of test-token allocated to the account in this test, set to a multiple of the max // amount deprecated / non-deprecated transactions commit to paying. pub const BALANCE: Fee = Fee(10
Move this definition after DEFAULT_ALL_BOUNDS_COMMITTED_FEE
Code quote:
BALANCE
crates/blockifier/src/test_utils.rs
line 122 at r1 (raw file):
pub const BALANCE: Fee = Fee(10 * const_max( const_max(DEFAULT_ALL_BOUNDS_COMMITTED_FEE.0, DEFAULT_L1_BOUNDS_COMMITTED_FEE.0),
Is there a case whereDEFAULT_ALL_BOUNDS_COMMITTED_FEE.0
< DEFAULT_L1_BOUNDS_COMMITTED_FEE.0
?
Code quote:
const_max(DEFAULT_ALL_BOUNDS_COMMITTED_FEE.0, DEFAULT_L1_BOUNDS_COMMITTED_FEE.0),
42b94c9
to
068214d
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.
Reviewable status: 9 of 13 files reviewed, 3 unresolved discussions (waiting on @yoavGrs)
crates/blockifier/src/test_utils.rs
line 100 at r1 (raw file):
// The max_fee / resource bounds used for txs in this test. // V3 transactions: pub const DEFAULT_L1_GAS_AMOUNT: GasAmount = GasAmount(1000000);
Done.
crates/blockifier/src/test_utils.rs
line 120 at r1 (raw file):
Previously, yoavGrs wrote…
Move this definition after
DEFAULT_ALL_BOUNDS_COMMITTED_FEE
Done.
crates/blockifier/src/test_utils.rs
line 122 at r1 (raw file):
Previously, yoavGrs wrote…
Is there a case where
DEFAULT_ALL_BOUNDS_COMMITTED_FEE.0
<DEFAULT_L1_BOUNDS_COMMITTED_FEE.0
?
in AllResourceBounds mode, it makes sense if the L1 gas bound is very low - even zero.
so, yes
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.
Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware)
Benchmark movements: |
This change is