test(sequencing): test sierra contract class serialization #3039
Conversation
|
This change is |
Yael-Starkware
force-pushed
the
yael/central_declare_transaction
branch
from
f60e0d3 to
4010674
Compare
Yael-Starkware
force-pushed
the
yael/central_sierra_contract_class
branch
from
2c04a0c to
440ed29
Compare
Yael-Starkware
changed the base branch from
yael/central_declare_transaction
to
main
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 227 at r1 (raw file):
} #[test]
Why not do the same for all objects? Why do we need to create the test object manually for other types?
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @Yael-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry and @DvirYo-starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 227 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Why not do the same for all objects? Why do we need to create the test object manually for other types?
The previous tests test also the into() functions.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @dafnamatsry)
Yael-Starkware
force-pushed
the
yael/central_sierra_contract_class
branch
from
440ed29 to
008e093
Compare
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
let serialized_sierra_contract_class = serde_json::to_value(&sierra_contract_class).unwrap(); assert_eq!(central_sierra_contract_class, serialized_sierra_contract_class);
What these 2 lines are testing?
It serializes an object that was created from a JSON, so it has to be that it will get serialized back to the same JSON.
I think it would be better to manually create a SierraContractClass instance, serialize it, and then compare.
Suggestion:
let serialized_sierra_contract_class = serde_json::to_value(&sierra_contract_class).unwrap();
assert_eq!(central_sierra_contract_class, serialized_sierra_contract_class);thancrates/sequencing/papyrus_consensus_orchestrator/resources/central_sierra_contract_class.json line 2286 at r1 (raw file):
"L1_HANDLER": [] }, "abi": "[\n {\n \"type\": \"impl\",\n \"name\": \"IERC20Impl\",\n \"interface_name\": \"cairo_level_tests::contracts::erc20::IERC20\"\n },\n {\n \"type\": \"struct\",\n \"name\": \"core::integer::u256\",\n \"members\": [\n {\n \"name\": \"low\",\n \"type\": \"core::integer::u128\"\n },\n {\n \"name\": \"high\",\n \"type\": \"core::integer::u128\"\n }\n ]\n },\n {\n \"type\": \"interface\",\n \"name\": \"cairo_level_tests::contracts::erc20::IERC20\",\n \"items\": [\n {\n \"type\": \"function\",\n \"name\": \"get_name\",\n \"inputs\": [],\n \"outputs\": [\n {\n \"type\": \"core::felt252\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"get_symbol\",\n \"inputs\": [],\n \"outputs\": [\n {\n \"type\": \"core::felt252\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"get_decimals\",\n \"inputs\": [],\n \"outputs\": [\n {\n \"type\": \"core::integer::u8\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"get_total_supply\",\n \"inputs\": [],\n \"outputs\": [\n {\n \"type\": \"core::integer::u256\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"balance_of\",\n \"inputs\": [\n {\n \"name\": \"account\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n }\n ],\n \"outputs\": [\n {\n \"type\": \"core::integer::u256\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"allowance\",\n \"inputs\": [\n {\n \"name\": \"owner\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"spender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n }\n ],\n \"outputs\": [\n {\n \"type\": \"core::integer::u256\"\n }\n ],\n \"state_mutability\": \"view\"\n },\n {\n \"type\": \"function\",\n \"name\": \"transfer\",\n \"inputs\": [\n {\n \"name\": \"recipient\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"amount\",\n \"type\": \"core::integer::u256\"\n }\n ],\n \"outputs\": [],\n \"state_mutability\": \"external\"\n },\n {\n \"type\": \"function\",\n \"name\": \"transfer_from\",\n \"inputs\": [\n {\n \"name\": \"sender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"recipient\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"amount\",\n \"type\": \"core::integer::u256\"\n }\n ],\n \"outputs\": [],\n \"state_mutability\": \"external\"\n },\n {\n \"type\": \"function\",\n \"name\": \"approve\",\n \"inputs\": [\n {\n \"name\": \"spender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"amount\",\n \"type\": \"core::integer::u256\"\n }\n ],\n \"outputs\": [],\n \"state_mutability\": \"external\"\n },\n {\n \"type\": \"function\",\n \"name\": \"increase_allowance\",\n \"inputs\": [\n {\n \"name\": \"spender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"added_value\",\n \"type\": \"core::integer::u256\"\n }\n ],\n \"outputs\": [],\n \"state_mutability\": \"external\"\n },\n {\n \"type\": \"function\",\n \"name\": \"decrease_allowance\",\n \"inputs\": [\n {\n \"name\": \"spender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n },\n {\n \"name\": \"subtracted_value\",\n \"type\": \"core::integer::u256\"\n }\n ],\n \"outputs\": [],\n \"state_mutability\": \"external\"\n }\n ]\n },\n {\n \"type\": \"constructor\",\n \"name\": \"constructor\",\n \"inputs\": [\n {\n \"name\": \"name_\",\n \"type\": \"core::felt252\"\n },\n {\n \"name\": \"symbol_\",\n \"type\": \"core::felt252\"\n },\n {\n \"name\": \"decimals_\",\n \"type\": \"core::integer::u8\"\n },\n {\n \"name\": \"initial_supply\",\n \"type\": \"core::integer::u256\"\n },\n {\n \"name\": \"recipient\",\n \"type\": \"core::starknet::contract_address::ContractAddress\"\n }\n ]\n },\n {\n \"type\": \"event\",\n \"name\": \"cairo_level_tests::contracts::erc20::erc_20::Transfer\",\n \"kind\": \"struct\",\n \"members\": [\n {\n \"name\": \"from\",\n \"type\": \"core::starknet::contract_address::ContractAddress\",\n \"kind\": \"data\"\n },\n {\n \"name\": \"to\",\n \"type\": \"core::starknet::contract_address::ContractAddress\",\n \"kind\": \"data\"\n },\n {\n \"name\": \"value\",\n \"type\": \"core::integer::u256\",\n \"kind\": \"data\"\n }\n ]\n },\n {\n \"type\": \"event\",\n \"name\": \"cairo_level_tests::contracts::erc20::erc_20::Approval\",\n \"kind\": \"struct\",\n \"members\": [\n {\n \"name\": \"owner\",\n \"type\": \"core::starknet::contract_address::ContractAddress\",\n \"kind\": \"data\"\n },\n {\n \"name\": \"spender\",\n \"type\": \"core::starknet::contract_address::ContractAddress\",\n \"kind\": \"data\"\n },\n {\n \"name\": \"value\",\n \"type\": \"core::integer::u256\",\n \"kind\": \"data\"\n }\n ]\n },\n {\n \"type\": \"event\",\n \"name\": \"cairo_level_tests::contracts::erc20::erc_20::Event\",\n \"kind\": \"enum\",\n \"variants\": [\n {\n \"name\": \"Transfer\",\n \"type\": \"cairo_level_tests::contracts::erc20::erc_20::Transfer\",\n \"kind\": \"nested\"\n },\n {\n \"name\": \"Approval\",\n \"type\": \"cairo_level_tests::contracts::erc20::erc_20::Approval\",\n \"kind\": \"nested\"\n }\n ]\n }\n]"
Can we format it to a nice JSON (without the \s)?
There was a problem hiding this comment.
Reviewable status: all files reviewed (commit messages unreviewed), 2 unresolved discussions (waiting on @dafnamatsry)
crates/sequencing/papyrus_consensus_orchestrator/resources/central_sierra_contract_class.json line 2286 at r1 (raw file):
Previously, dafnamatsry wrote…
Can we format it to a nice JSON (without the
\s)?
this is a real contract abi string.
The field type is String , so it can't be a nice json.
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
Previously, dafnamatsry wrote…
What these 2 lines are testing?
It serializes an object that was created from a JSON, so it has to be that it will get serialized back to the same JSON.I think it would be better to manually create a SierraContractClass instance, serialize it, and then compare.
The first deserialization is in order to create a sierra_contract_class_object.
the second is in order to check that the serialization of the object creates the same json as was serialized from python.
in general serialization and deserialization of an object doesn't necessarily have the use and result with the same json.
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/resources/central_sierra_contract_class.json line 2286 at r1 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
this is a real contract abi string.
The field type is String , so it can't be a nice json.
The \n can be removed.
Not critical, only if you feel like changing it...
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
The first deserialization is in order to create a sierra_contract_class_object.
the second is in order to check that the serialization of the object creates the same json as was serialized from python.in general serialization and deserialization of an object doesn't necessarily have the use and result with the same json.
Say tomorrow someone adds an optional field to SierraContractClass (with skip serialise if none, like in CasmContractClass), this test will still pass. I would have want it to fail in this case.
Yael-Starkware
force-pushed
the
yael/central_sierra_contract_class
branch
from
008e093 to
f5dee18
Compare
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @dafnamatsry)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
Previously, dafnamatsry wrote…
Say tomorrow someone adds an optional field to
SierraContractClass(with skip serialise if none, like inCasmContractClass), this test will still pass. I would have want it to fail in this case.
It is the same for all central objects,
if someone adds a field to an object, they need to add it to the entire pipeline, both rust and python and test end to end that it is passed to the pythonic services.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Yael-Starkware)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
It is the same for all central objects,
if someone adds a field to an object, they need to add it to the entire pipeline, both rust and python and test end to end that it is passed to the pythonic services.
In the other central objects tests, you create an instance manually (not by deserialize it from JSON). You have a rust_json, and a python_json which you compare.
Here, you begin with the expected json, and the test is less hermetic. As an example:
In the other cases, adding a new field to an object will make the test not compile, which is good, since then it must be adjusted to the changes.
Here adding an optional field for example, won't affect the test at all.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dafnamatsry)
crates/sequencing/papyrus_consensus_orchestrator/src/cende/central_objects_test.rs line 283 at r2 (raw file):
Previously, dafnamatsry wrote…
In the other central objects tests, you create an instance manually (not by deserialize it from JSON). You have a rust_json, and a python_json which you compare.
Here, you begin with the expected json, and the test is less hermetic. As an example:
In the other cases, adding a new field to an object will make the test not compile, which is good, since then it must be adjusted to the changes.
Here adding an optional field for example, won't affect the test at all.
- it is still not hermetic since you can make the test compile even without adding this field to the central object.
- even if I manually create this object, I won't be able to manually create the transaction_execution_info object since it has some private fields.
Yael-Starkware
force-pushed
the
yael/central_sierra_contract_class
branch
from
f5dee18 to
16a663a
Compare
Yael-Starkware
force-pushed
the
yael/central_sierra_contract_class
branch
from
16a663a to
830923a
Compare
No description provided.