feat(blockifier): add get_execution_info_v2 syscall
#1812
Conversation
varex83
added
the
native integration
|
This change is |
|
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.
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.
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.
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…
TryIntois not implemented in this case for the enum, but corresponding integers are defined in the enum itself, so converting withasis 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.
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};
varex83
force-pushed
the
bohdan/get-execution-info
branch
2 times, most recently
from
2efef80
to
70aaed6
Compare
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
2 times, most recently
from
984af55
to
7e1fe06
Compare
|
Artifacts upload triggered. View details here |
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
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.
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)
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
7e1fe06
to
af2de44
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
varex83
force-pushed
the
bohdan/get-execution-info
branch
2 times, most recently
from
5af9c72
to
82c7715
Compare
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
af2de44
to
1738c76
Compare
|
Artifacts upload triggered. View details here |
varex83
force-pushed
the
bohdan/get-execution-info
branch
from
82c7715
to
f246924
Compare
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
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.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status:
varex83
force-pushed
the
bohdan/get-execution-info
branch
3 times, most recently
from
ffd093d
to
f3a504e
Compare
There was a problem hiding this comment.
Reviewable status:
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
b6988aa
to
f1bbb57
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status:
There was a problem hiding this comment.
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:
varex83
force-pushed
the
bohdan/get-execution-info
branch
2 times, most recently
from
7f6d275
to
e75ea9a
Compare
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
f1bbb57
to
fd49a8b
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
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
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
fd49a8b
to
5d1f6c5
Compare
There was a problem hiding this comment.
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.
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:
There was a problem hiding this comment.
Reviewed 1 of 1 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status:
varex83
force-pushed
the
bohdan/get-execution-info-v2
branch
from
5d1f6c5
to
8f17982
Compare
|
Artifacts upload triggered. View details here |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status:
No description provided.