feat(papyrus_storage): get versioned casm #2420
Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
AvivYossef-starkware
requested review from
alonh5,
dorimedini-starkware,
DvirYo-starkware,
TzahiTaub and
Yoni-Starkware
and removed request for
alonh5
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
4466854 to
c8f3cb5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2420 +/- ##
===========================================
+ Coverage 40.10% 77.38% +37.28%
===========================================
Files 26 396 +370
Lines 1895 42683 +40788
Branches 1895 42683 +40788
===========================================
+ Hits 760 33031 +32271
- Misses 1100 7353 +6253
- Partials 35 2299 +2264 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @AvivYossef-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 66 at r1 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and his sierra version given its Sierra class hash.
Suggestion:
itscrates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
Suggestion:
match (self.get_casm(class_hash)?, self.get_class(class_hash)?) {
(Some(casm), Some(sierra)) => {
let sierra_version = SierraVersion::extract_from_program(&sierra.sierra_program)?;
Ok(Some((casm, sierra_version)))
}
(_, _) => Ok(None)
}crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
@AvivYossef-starkware will this function be called in the blockifier for every entry point in a running tx?
if so, @Yoni-Starkware is this an ok performance hit?
Code quote:
fn get_versioned_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<VersionedCasm>> {
// TODO(Aviv): Consider adding a dedicated table to store Sierra versions for better
// organization and performance.
if let Some(casm) = self.get_casm(class_hash)? {
if let Some(sierra) = self.get_class(class_hash)? {
let sierra_version = SierraVersion::extract_from_program(&sierra.sierra_program)?;
return Ok(Some((casm, sierra_version)));
}
}
Ok(None)crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
serde_json::from_value(serde_json::Value::deserialize_from(bytes)?).ok() } }
- this is not a primitive type, why is it in the primitive types section?
- why not use
auto_storage_serde!?
Code quote:
impl StorageSerde for SierraVersion {
fn serialize_into(&self, res: &mut impl std::io::Write) -> Result<(), StorageSerdeError> {
serde_json::to_value(self)?.serialize_into(res)
}
fn deserialize_from(bytes: &mut impl std::io::Read) -> Option<Self> {
serde_json::from_value(serde_json::Value::deserialize_from(bytes)?).ok()
}
}
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
c8f3cb5 to
002ea4b
Compare
There was a problem hiding this comment.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
Previously, dorimedini-starkware wrote…
@AvivYossef-starkware will this function be called in the blockifier for every entry point in a running tx?
if so, @Yoni-Starkware is this an ok performance hit?
No, it called only after Chache miss,
This struct has a cache of
RunnableCompiledClass and I'll add it also the SierraVersion
crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
Previously, dorimedini-starkware wrote…
- this is not a primitive type, why is it in the primitive types section?
- why not use
auto_storage_serde!?
For using auto_storage_serde! I should copy the struct definition.
crates/papyrus_storage/src/compiled_class.rs line 66 at r1 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and his sierra version given its Sierra class hash.
Done.
crates/papyrus_storage/src/compiled_class.rs line 101 at r1 (raw file):
} } Ok(None)
Done.
There was a problem hiding this comment.
You need to support get_sierra
Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
@avi-starkware FYI
Reviewable status: 3 of 4 files reviewed, 4 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/papyrus_storage/src/serialization/serializers.rs line 756 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
For using
auto_storage_serde!I should copy the struct definition.
this is the standard practice in this module iiuc
AvivYossef-starkware
force-pushed
the
aviv/add_sierra_version
branch
2 times, most recently
from
1bbe7f1 to
aa5b760
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
e0a4c01 to
865d1dc
Compare
There was a problem hiding this comment.
Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
crates/papyrus_storage/src/compiled_class.rs line 66 at r3 (raw file):
/// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the Cairo assembly of a class and its sierra contract class given its Sierra class hash.
Suggestion:
d its Sierra con
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
865d1dc to
2fdc7e4
Compare
AvivYossef-starkware
changed the base branch from
aviv/add_sierra_version
to
aviv/refactor_default_sierra_contract_class
There was a problem hiding this comment.
Reviewed 1 of 6 files at r14, 1 of 3 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class_test.rs line 46 at r15 (raw file):
let ((reader, mut writer), _temp_dir) = get_test_storage(); let expected_casm = CasmContractClass::get_test_instance(&mut rng); let expected_sierra = <SierraContractClass as GetTestInstance>::get_test_instance(&mut rng);
Why do you need the <SierraContractClass as GetTestInstance>? I think doing the same as in the CASM case will be enough
crates/papyrus_storage/src/compiled_class.rs line 65 at r15 (raw file):
pub trait CasmStorageReader { /// Returns the Cairo assembly of a class given its Sierra class hash. fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>;
Add a new line here
crates/papyrus_storage/src/compiled_class.rs line 67 at r15 (raw file):
fn get_casm(&self, class_hash: &ClassHash) -> StorageResult<Option<CasmContractClass>>; /// Returns the CASM and Sierra contract classes for the given hash. /// If both exist, returns `(Some(casm), Some(sierra))`. If neither, returns `(None, None)`.
Consider each if in a new line
crates/papyrus_storage/src/compiled_class.rs line 97 at r15 (raw file):
} fn get_casm_and_sierra(
Just to make it clear, the call
reader.get_casm_and_sierra(class_hash)?
is totally equivalent to
(reader.get_casm(class_hash)?, reader.get_sierra(class_hash)?)
If I understand it correctly, I don't see any reason for this function. @Yoni-Starkware, if you approve, I'm fine with adding this.
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
ceffe13 to
12d69a6
Compare
There was a problem hiding this comment.
Reviewable status: 9 of 11 files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, @TzahiTaub, and @Yoni-Starkware)
crates/papyrus_storage/src/compiled_class.rs line 97 at r15 (raw file):
Previously, DvirYo-starkware wrote…
Just to make it clear, the call
reader.get_casm_and_sierra(class_hash)?
is totally equivalent to
(reader.get_casm(class_hash)?, reader.get_sierra(class_hash)?)If I understand it correctly, I don't see any reason for this function. @Yoni-Starkware, if you approve, I'm fine with adding this.
Yes, its the same.
crates/papyrus_storage/src/compiled_class_test.rs line 46 at r15 (raw file):
Previously, DvirYo-starkware wrote…
Why do you need the
<SierraContractClass as GetTestInstance>? I think doing the same as in the CASM case will be enough
The compiler is uncertain whether to use the function defined in GetTestInstance or another option. Therefore, I needed to add it.
Yoni-Starkware
requested review from
dorimedini-starkware,
DvirYo-starkware and
Yoni-Starkware
There was a problem hiding this comment.
Reviewed 1 of 6 files at r14, 2 of 3 files at r15, 1 of 1 files at r16.
Reviewable status: 10 of 11 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @DvirYo-starkware, and @TzahiTaub)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware and @TzahiTaub)
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TzahiTaub and @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status:
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
12d69a6 to
1236dca
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
4471084 to
6172e4d
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r18, all commit messages.
Reviewable status:
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
2 times, most recently
from
a066e45 to
31dda01
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
31dda01 to
9f4d9e1
Compare
|
Benchmark movements: |
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
9f4d9e1 to
422c84e
Compare
There was a problem hiding this comment.
Reviewable status:
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
422c84e to
925eedd
Compare
AvivYossef-starkware
force-pushed
the
aviv/get_versioned_casm
branch
from
925eedd to
8a0b219
Compare
No description provided.