-
Notifications
You must be signed in to change notification settings - Fork 26
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
test(blockifier): execute_directly with v3 invoke instead of deprecated #1770
test(blockifier): execute_directly with v3 invoke instead of deprecated #1770
Conversation
8f28207
to
59b4d31
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
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 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)
crates/blockifier/src/test_utils/struct_impls.rs
line 58 at r1 (raw file):
let default_invoke_tx = account_invoke_tx(InvokeTxArgs::default()); let tx_info = default_invoke_tx.create_tx_info(); assert!(matches!(tx_info, TransactionInfo::Current(_)));
this is an awkward way to get the desired default tx context...
maybe add a &TransactionInfo
input to this method? is it possible?
Code quote:
let limit_steps_by_resources = false;
let default_invoke_tx = account_invoke_tx(InvokeTxArgs::default());
let tx_info = default_invoke_tx.create_tx_info();
assert!(matches!(tx_info, TransactionInfo::Current(_)));
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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/blockifier/src/test_utils/struct_impls.rs
line 58 at r1 (raw file):
Previously, dorimedini-starkware wrote…
this is an awkward way to get the desired default tx context...
maybe add a&TransactionInfo
input to this method? is it possible?
But I want to save the callers this boilerplate code
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1770 +/- ##
===========================================
+ Coverage 40.10% 68.54% +28.43%
===========================================
Files 26 102 +76
Lines 1895 13625 +11730
Branches 1895 13625 +11730
===========================================
+ Hits 760 9339 +8579
- Misses 1100 3885 +2785
- Partials 35 401 +366 ☔ View full report in Codecov by Sentry. |
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: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)
crates/blockifier/src/test_utils/struct_impls.rs
line 58 at r1 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
But I want to save the callers this boilerplate code
add CurrentTransactionInfo::default_for_testing
59b4d31
to
b1cd0aa
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: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @TzahiTaub)
crates/blockifier/src/test_utils/struct_impls.rs
line 58 at r1 (raw file):
Previously, dorimedini-starkware wrote…
add
CurrentTransactionInfo::default_for_testing
Done.
Artifacts upload triggered. View details here |
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 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TzahiTaub)
crates/blockifier/src/transaction/objects.rs
line 126 at r2 (raw file):
Self { common_fields: CommonAccountFields::default(), resource_bounds: ValidResourceBounds::create_for_testing(),
this gives AllResources that are nonzero, right?
Code quote:
ValidResourceBounds::create_for_testing()
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: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/blockifier/src/transaction/objects.rs
line 126 at r2 (raw file):
Previously, dorimedini-starkware wrote…
this gives AllResources that are nonzero, right?
GasAmount for L2 is high, price is 0, and for the other two, amount is 1 and price is 0. This is to keep enforce_fee==False.
99ab043
to
f25405d
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: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
Artifacts upload triggered. View details here |
b1cd0aa
to
44c0f5a
Compare
Artifacts upload triggered. View details here |
44c0f5a
to
a20c055
Compare
Artifacts upload triggered. View details here |
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 1 of 1 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub)
No description provided.