-
Notifications
You must be signed in to change notification settings - Fork 27
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
chore(blockifier): compute valid retdata directly #2113
Conversation
Artifacts upload triggered. View details here |
Artifacts upload triggered. View details here |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2113 +/- ##
===========================================
+ Coverage 40.10% 77.28% +37.18%
===========================================
Files 26 108 +82
Lines 1895 13948 +12053
Branches 1895 13948 +12053
===========================================
+ Hits 760 10780 +10020
- Misses 1100 2707 +1607
- Partials 35 461 +426 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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 3 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware, @dorimedini-starkware, and @MohammadNassar1)
crates/blockifier/src/transaction/constants.rs
line 21 at r1 (raw file):
// Expected return value of a `validate` entry point: `VALID`. pub static VALIDATE_RETDATA: LazyLock<Felt> = LazyLock::new(|| ascii_as_felt("VALID").expect("Failed to parse ASCII"));
why is the lock needed?
Previously, ayeletstarkware (Ayelet Zilber) wrote…
Here is what I modeled after. |
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 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dorimedini-starkware and @MohammadNassar1)
d0bfc50
to
4d59141
Compare
Artifacts upload triggered. View details here |
Benchmark movements: full_committer_flow performance regressed! |
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 and @MohammadNassar1)
crates/blockifier/src/transaction/constants.rs
line 21 at r1 (raw file):
Previously, ArniStarkware (Arnon Hod) wrote…
Here is what I modeled after.
Don't you want to remove it and use starknet_api
's VALIDATE_RETDATA
?
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 @dorimedini-starkware and @MohammadNassar1)
crates/blockifier/src/transaction/constants.rs
line 21 at r1 (raw file):
Previously, ayeletstarkware (Ayelet Zilber) wrote…
Don't you want to remove it and use
starknet_api
'sVALIDATE_RETDATA
?
Yes. The comment thread is now on a deleted file. The change appears in crates/starknet_api/src/transaction/constants.rs
on this commit.
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 @dorimedini-starkware and @MohammadNassar1)
Created to address a thought on: #2104.