refactor(starknet_api): add contract class version to sierra contract #2277
Conversation
|
This change is |
|
Benchmark movements: full_committer_flow performance regressed! |
AvivYossef-starkware
force-pushed
the
aviv/refactor_sn_api_contract_class_name
branch
2 times, most recently
from
975b893 to
90127f0
Compare
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
1e1106a to
08e4ae7
Compare
|
Benchmark movements: |
AvivYossef-starkware
force-pushed
the
aviv/refactor_sn_api_contract_class_name
branch
2 times, most recently
from
d7f67f0 to
8b84db9
Compare
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
08e4ae7 to
dba785d
Compare
|
Benchmark movements: |
AvivYossef-starkware
changed the base branch from
aviv/refactor_sn_api_contract_class_name
to
graphite-base/2277
AvivYossef-starkware
force-pushed
the
graphite-base/2277
branch
from
8b84db9 to
fcf5cce
Compare
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
dba785d to
d9a5dec
Compare
AvivYossef-starkware
changed the base branch from
graphite-base/2277
to
main
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
d9a5dec to
851feac
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)
crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):
)?, abi: String::deserialize_from(&mut decompress_from_reader(bytes)?.as_slice())?, contract_class_version: String::deserialize_from(bytes)?,
Seems like you need to keep the order here
Code quote:
fn deserialize_from(bytes: &mut impl std::io::Read) -> Option<Self> {
Some(Self {
sierra_program: Vec::<Felt>::deserialize_from(
&mut decompress_from_reader(bytes)?.as_slice(),
)?,
entry_points_by_type: HashMap::<EntryPointType, Vec<EntryPoint>>::deserialize_from(
bytes,
)?,
abi: String::deserialize_from(&mut decompress_from_reader(bytes)?.as_slice())?,
contract_class_version: String::deserialize_from(bytes)?,crates/starknet_api/src/state.rs line 218 at r1 (raw file):
pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>, pub abi: String, pub contract_class_version: String,
To be consistent with the compiler's struct
Suggestion:
pub sierra_program: Vec<Felt>,
pub contract_class_version: String,
pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
pub abi: String,crates/papyrus_storage/src/utils_test.rs line 34 at r1 (raw file):
ClassHash(i_felt), SierraContractClass { contract_class_version: "".to_string(),
Let it be "0.1.0"
Code quote:
"".to_string(),There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)
crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Seems like you need to keep the order here
the order of the fields?
|
Benchmark movements: full_committer_flow performance regressed! |
There was a problem hiding this comment.
Are you removing the duplicated object in a separate PR?
Reviewed all commit messages.
Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @AvivYossef-starkware)
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
851feac to
2295fe7
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 3 unresolved discussions (waiting on @Yoni-Starkware)
crates/papyrus_storage/src/utils_test.rs line 34 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Let it be "0.1.0"
Done.
crates/starknet_api/src/state.rs line 218 at r1 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
To be consistent with the compiler's struct
Done.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2277 +/- ##
===========================================
+ Coverage 40.10% 61.52% +21.41%
===========================================
Files 26 85 +59
Lines 1895 10354 +8459
Branches 1895 10354 +8459
===========================================
+ Hits 760 6370 +5610
- Misses 1100 3056 +1956
- Partials 35 928 +893 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @AvivYossef-starkware)
crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):
#[derive(Serialize)] struct DumpDeclaredClass {
Can you ask @DvirYo-starkware what is this class and why it's missing the string ABI?
Code quote:
#[derive(Serialize)]
struct DumpDeclaredClass {crates/papyrus_storage/src/utils.rs line 31 at r2 (raw file):
compiled_class_hash: CompiledClassHash, sierra_program: Vec<Felt>, entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
Keep the order consistent
Suggestion:
struct DumpDeclaredClass {
class_hash: ClassHash,
compiled_class_hash: CompiledClassHash,
sierra_program: Vec<Felt>,
contract_class_version: String,
entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,crates/papyrus_storage/src/utils.rs line 82 at r2 (raw file):
compiled_class_hash: *compiled_class_hash, sierra_program: contract_class.sierra_program.clone(), entry_points_by_type: contract_class.entry_points_by_type.clone(),
Suggestion:
&DumpDeclaredClass {
class_hash: *class_hash,
compiled_class_hash: *compiled_class_hash,
sierra_program: contract_class.sierra_program.clone(),
contract_class_version: contract_class.contract_class_version.clone(),
entry_points_by_type: contract_class.entry_points_by_type.clone(),
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @AvivYossef-starkware)
crates/papyrus_storage/src/serialization/serializers.rs line 986 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
the order of the fields?
Yes
crates/starknet_client/src/reader/objects/state.rs line 71 at r2 (raw file):
contract_class_version: class.contract_class_version, sierra_program: class.sierra_program, entry_points_by_type: class.entry_points_by_type,
Suggestion:
Self {
sierra_program: class.sierra_program,
contract_class_version: class.contract_class_version,
entry_points_by_type: class.entry_points_by_type,crates/papyrus_test_utils/src/lib.rs line 503 at r2 (raw file):
pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>, pub abi: String, pub contract_class_version: String,
Suggestion:
pub struct SierraContractClass {
pub sierra_program: Vec<Felt>,
pub contract_class_version: String,
pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>,
pub abi: String,
|
Benchmark movements: |
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
2295fe7 to
3893a27
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 7 files reviewed, 5 unresolved discussions (waiting on @Yoni-Starkware)
crates/papyrus_storage/src/utils.rs line 31 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Keep the order consistent
Done.
crates/papyrus_storage/src/utils.rs line 82 at r2 (raw file):
compiled_class_hash: *compiled_class_hash, sierra_program: contract_class.sierra_program.clone(), entry_points_by_type: contract_class.entry_points_by_type.clone(),
Done.
crates/papyrus_test_utils/src/lib.rs line 503 at r2 (raw file):
pub entry_points_by_type: HashMap<EntryPointType, Vec<EntryPoint>>, pub abi: String, pub contract_class_version: String,
Done.
crates/starknet_client/src/reader/objects/state.rs line 71 at r2 (raw file):
contract_class_version: class.contract_class_version, sierra_program: class.sierra_program, entry_points_by_type: class.entry_points_by_type,
Done.
|
Benchmark movements: |
There was a problem hiding this comment.
Don't forget to delete RpcContractClass
Reviewed 1 of 6 files at r1, 2 of 4 files at r2, 4 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware)
crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Can you ask @DvirYo-starkware what is this class and why it's missing the string ABI?
Non blocking
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1, 3 of 4 files at r2, 3 of 4 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware)
crates/papyrus_storage/src/utils.rs line 26 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Non blocking
The struct DumpDeclaredClass can be deleted. I will delete it in a different PR.
crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):
fn serialize_into(&self, res: &mut impl std::io::Write) -> Result<(), StorageSerdeError> { serialize_and_compress(&self.sierra_program)?.serialize_into(res)?; self.contract_class_version.serialize_into(res)?;
This is a breaking change to the storage. We should discuss this.
A few months ago, we decided to break the storage in version 13.3. I don't know what was agreed upon about the storage in 13.3 and 13.4, but it is fine if we break the storage in 13.4.
crates/papyrus_protobuf/src/converters/class.rs line 209 at r3 (raw file):
let sierra_program = value.program.into_iter().map(Felt::try_from).collect::<Result<Vec<_>, _>>()?;
Make sure with @ShahakShama it is fine (I don't think should be a problem here)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)
crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):
Previously, DvirYo-starkware wrote…
This is a breaking change to the storage. We should discuss this.
A few months ago, we decided to break the storage in version 13.3. I don't know what was agreed upon about the storage in 13.3 and 13.4, but it is fine if we break the storage in 13.4.
I think that we initialize papyrus with each new version, right? @Yoni-Starkware
There was a problem hiding this comment.
Just two more PRS to reach it.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yoni-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @DvirYo-starkware)
crates/papyrus_storage/src/serialization/serializers.rs line 972 at r3 (raw file):
Previously, AvivYossef-starkware wrote…
I think that we initialize papyrus with each new version, right? @Yoni-Starkware
@AvivYossef-starkware not necessarily but we would have to do this for this version since we need all the Sierra classes. So LGTM
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware and @DvirYo-starkware)
crates/papyrus_protobuf/src/converters/class.rs line 209 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Make sure with @ShahakShama it is fine (I don't think should be a problem here)
Now the conversion to protobuf isn't the inverse of the conversion from protobuf. Fix the other conversion (I'll put a comment there
crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):
}); let contract_class_version = format!(
Remove this, and extract the contract_class_version from value instead
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
3893a27 to
2333708
Compare
There was a problem hiding this comment.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @DvirYo-starkware, @ShahakShama, and @Yoni-Starkware)
crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):
Previously, ShahakShama wrote…
Remove this, and extract the contract_class_version from value instead
Thanks, consider adding a test to enforce this.
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
crates/papyrus_protobuf/src/converters/class.rs line 292 at r3 (raw file):
Previously, AvivYossef-starkware wrote…
Thanks, consider adding a test to enforce this.
There's a TODO to add that test (FYI @noamsp-starkware )
AvivYossef-starkware
force-pushed
the
aviv/refactor_sierra_contract_class
branch
from
2333708 to
15764ce
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
No description provided.