-
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
feat(blockifier): add get_execution_info_v2
syscall
#1812
Conversation
Artifacts upload triggered. View details here |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1812 +/- ##
===========================================
+ Coverage 40.10% 77.12% +37.02%
===========================================
Files 26 105 +79
Lines 1895 13663 +11768
Branches 1895 13663 +11768
===========================================
+ Hits 760 10538 +9778
- Misses 1100 2667 +1567
- Partials 35 458 +423 ☔ 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.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @varex83)
crates/blockifier/src/execution/native/syscall_handler.rs
line 203 at r1 (raw file):
paymaster_data: context.paymaster_data.0.clone(), nonce_data_availability_mode: context.nonce_data_availability_mode as u32, fee_data_availability_mode: context.fee_data_availability_mode as u32,
Please use into or try_ino and not as
Suggestion:
nonce_data_availability_mode: context.nonce_data_availability_mode.into(),
fee_data_availability_mode: context.fee_data_availability_mode.into(),
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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/native/syscall_handler.rs
line 203 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Please use into or try_ino and not as
TryInto
is not implemented in this case for the enum, but corresponding integers are defined in the enum itself, so converting with as
is a pretty idiomatic way to do
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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/native/syscall_handler.rs
line 203 at r1 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
TryInto
is not implemented in this case for the enum, but corresponding integers are defined in the enum itself, so converting withas
is a pretty idiomatic way to do
Oh, just noticed that clippy failed (I was checking it a few times, but only now it appeared). I will push fix ASAP
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 2 files at r2.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @noaov1 and @varex83)
crates/blockifier/src/execution/native/syscall_handler.rs
line 20 at r1 (raw file):
use starknet_types_core::felt::Felt; use super::utils::{calculate_resource_bounds, default_tx_v2_info};
Avoid using super
Code quote:
use super::utils::{calculate_resource_bounds, default_tx_v2_info};
2efef80
to
70aaed6
Compare
984af55
to
7e1fe06
Compare
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.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/native/syscall_handler.rs
line 20 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Avoid using super
Done
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 1 of 2 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)
7e1fe06
to
af2de44
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 r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)
5af9c72
to
82c7715
Compare
af2de44
to
1738c76
Compare
Artifacts upload triggered. View details here |
82c7715
to
f246924
Compare
1738c76
to
7c6e4e3
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
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 1 of 1 files at r8, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)
ffd093d
to
f3a504e
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 @noaov1 and @Yoni-Starkware)
b6988aa
to
f1bbb57
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 r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)
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 2 files at r2, 2 of 4 files at r6, 1 of 1 files at r7, 1 of 1 files at r8.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)
7f6d275
to
e75ea9a
Compare
f1bbb57
to
fd49a8b
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.
Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @meship-starkware, @noaov1, and @varex83)
crates/blockifier/src/execution/native/utils.rs
line 72 at r10 (raw file):
}, ResourceBounds { resource: Felt::from_hex(Resource::L1DataGas.to_hex()).unwrap(),
I think this is the wrong order. Please check against the vm impl
fd49a8b
to
5d1f6c5
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: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @meship-starkware, @noaov1, and @Yoni-Starkware)
crates/blockifier/src/execution/native/utils.rs
line 72 at r10 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
I think this is the wrong order. Please check against the vm impl
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 1 of 2 files at r2, 1 of 4 files at r6, 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)
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 r10, 1 of 1 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)
5d1f6c5
to
8f17982
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 r12, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noaov1)
No description provided.