-
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 replace_class cairo native syscall #1551
Conversation
Artifacts upload triggered. View details here |
b55f2d1
to
d16b6c7
Compare
Artifacts upload triggered. View details here |
d16b6c7
to
74a67fe
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.
Please, rebase / change target branch to #1305
Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @PearsonWhite)
crates/blockifier/src/execution/native/syscall_handler.rs
line 180 at r3 (raw file):
.state .get_compiled_contract_class(class_hash) .expect("Failed to get ContractClass from class hash: {class_hash}.");
Please, be consistent with VM implementation, return the runtime error here, not panic
crates/blockifier/src/execution/native/syscall_handler.rs
line 186 at r3 (raw file):
let err = &SyscallExecutionError::ForbiddenClassReplacement { class_hash }.to_string(); panic!("{err:?}");
Don't use panic here
crates/blockifier/src/execution/native/syscall_handler.rs
line 191 at r3 (raw file):
self.state .set_class_hash_at(self.contract_address, class_hash) .expect("Failed to set class hash for class hash: {class_hash}.");
And here
a00994e
to
e2c9fe8
Compare
12eb020
to
4a2d661
Compare
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1551 +/- ##
===========================================
+ Coverage 40.10% 69.04% +28.93%
===========================================
Files 26 105 +79
Lines 1895 13665 +11770
Branches 1895 13665 +11770
===========================================
+ Hits 760 9435 +8675
- Misses 1100 3827 +2727
- Partials 35 403 +368 ☔ 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.
I rebased it onto rdr/add-syscall-counting
because it doesn't depend on rdr/native-storage-read
.
Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @varex83)
crates/blockifier/src/execution/native/syscall_handler.rs
line 180 at r3 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
Please, be consistent with VM implementation, return the runtime error here, not panic
Done.
crates/blockifier/src/execution/native/syscall_handler.rs
line 186 at r3 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
Don't use panic here
Done.
crates/blockifier/src/execution/native/syscall_handler.rs
line 191 at r3 (raw file):
Previously, varex83 (Bohdan Ohorodnii) wrote…
And here
Done.
147c1d3
to
ded98cf
Compare
4a2d661
to
1fc159e
Compare
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
8bc0df3
to
6108edb
Compare
Artifacts upload triggered. View details here |
ded98cf
to
6427073
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.
Reviewed 1 of 5 files at r4, 5 of 6 files at r5, all commit messages.
Reviewable status: 6 of 7 files reviewed, 3 unresolved discussions (waiting on @noaov1 and @varex83)
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 r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1)
c8574ed
to
dccd9c5
Compare
Artifacts upload triggered. View details here |
dccd9c5
to
6a12c44
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.
Reviewed 4 of 4 files at r10, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1)
5a001b3
to
d90688f
Compare
6a12c44
to
b7222f2
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 4 files at r10, 1 of 1 files at r11, 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 1 files at r11, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1 and @Yoni-Starkware)
816c81b
to
b0fa3a9
Compare
b7222f2
to
8a2e742
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 2 files at r12, all commit messages.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @avi-starkware, @noaov1, @PearsonWhite, and @Yoni-Starkware)
crates/blockifier/src/execution/syscalls/syscall_tests/replace_class.rs
line 60 at r12 (raw file):
#[cfg_attr( feature = "cairo_native", test_case(FeatureContract::TestContract(CairoVersion::Native), 13850; "Native")
The previous gas price was the right price. I believe it will be fixed after the update.
Code quote:
test_case(FeatureContract::TestContract(CairoVersion::Native), 13850; "Native")
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 r12.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1, @PearsonWhite, 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 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @noaov1 and @PearsonWhite)
Implements the
replace_class
syscall for native cairo syscall handler.