-
Notifications
You must be signed in to change notification settings - Fork 85
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(JSON-RPC): add deprecated contract class to getCompiledContractC… #2093
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2093 +/- ##
==========================================
+ Coverage 68.06% 68.30% +0.23%
==========================================
Files 132 132
Lines 17665 17615 -50
Branches 17665 17615 -50
==========================================
+ Hits 12024 12032 +8
+ Misses 4311 4253 -58
Partials 1330 1330 ☔ 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.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @Yael-Starkware)
a discussion (no related file):
The contract could be a big object. You should consider maybe compressing it before sending it to the network
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1453 at r1 (raw file):
} if let Some(casm) = storage_txn.get_casm(&class_hash).map_err(internal_server_error)? {
Why did you change the error type?
Code quote:
internal_server_error
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1460 at r1 (raw file):
.ok_or_else(|| ErrorObjectOwned::from(CLASS_HASH_NOT_FOUND))?; let deprecated_compiled_contract_class = storage_txn .get_state_reader()
Consider creating state reader only once
Code quote:
.get_state_reader()
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3574 at r1 (raw file):
let casm_class_hash = ClassHash(stark_felt!("0x1")); let casm_contract_class = CasmContractClass::get_test_instance(&mut get_rng()); let deprecated_class_hash = ClassHash(stark_felt!("0x2"));
Consider using the constant of felt
Code quote:
stark_felt!("0x2")
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3585 at r1 (raw file):
declared_classes: IndexMap::from([ (casm_class_hash, CompiledClassHash::default()), (deprecated_class_hash, CompiledClassHash::default()),
There is a different section in the state diff for deprecated classes
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: all files reviewed, 4 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
a discussion (no related file):
Previously, DvirYo-starkware wrote…
The contract could be a big object. You should consider maybe compressing it before sending it to the network
Currently we use the rpc call only for testing. the actual product will work with p2p so no need to optimize atm.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1453 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Why did you change the error type?
There was a confusion about the way the class hash table works.
I think now it should be fixed.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1460 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Consider creating state reader only once
Done.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3574 at r1 (raw file):
Previously, DvirYo-starkware wrote…
Consider using the constant of felt
done.
people here don't like consts inside function but it seems fit to me, let me know what you think.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3585 at r1 (raw file):
Previously, DvirYo-starkware wrote…
There is a different section in the state diff for deprecated classes
oh that explains a lot! I thought I found a bug in the storage reader! but now it makes sense.
I hope now it's right.
4abd393
to
a1275f3
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.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3571 at r2 (raw file):
const DEPRECATED_CLASS_HASH: ClassHash = ClassHash(StarkFelt::from_u128(2)); const INVALID_CLASS_HASH: ClassHash = ClassHash(StarkFelt::from_u128(3));
not sure how you feel about constants inside a function, do you think it's batter to change it to regular variable?
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: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3571 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
not sure how you feel about constants inside a function, do you think it's batter to change it to regular variable?
*better
a1275f3
to
8dfd3ed
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.
Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3571 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
*better
actually it looked bad , so I made them variables.
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 r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dan-starkware and @Yael-Starkware)
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1444 at r3 (raw file):
) -> RpcResult<CompiledContractClass> { let storage_txn = self.storage_reader.begin_ro_txn().map_err(internal_server_error)?; let state_reader = storage_txn.get_state_reader().map_err(internal_server_error)?;
Do it in one line
Code quote:
let storage_txn = self.storage_reader.begin_ro_txn().map_err(internal_server_error)?;
let state_reader = storage_txn.get_state_reader().map_err(internal_server_error)?;
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1446 at r3 (raw file):
let state_reader = storage_txn.get_state_reader().map_err(internal_server_error)?; let block_number = get_accepted_block_number(&storage_txn, block_id)?; if let Some(class_definition_block_number) = state_reader
Consider adding a comment here to say that this is the logic for cairo1
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1461 at r3 (raw file):
let state_number = StateNumber::right_after_block(block_number) .ok_or_else(|| ErrorObjectOwned::from(CLASS_HASH_NOT_FOUND))?;
This error should be internal_server_error
. The fact we fail to get the state after the block has nothing to do with the CLASS_HASH_NOT_FOUND error.
Code quote:
.ok_or_else(|| ErrorObjectOwned::from(CLASS_HASH_NOT_FOUND))?;
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1462 at r3 (raw file):
let state_number = StateNumber::right_after_block(block_number) .ok_or_else(|| ErrorObjectOwned::from(CLASS_HASH_NOT_FOUND))?; let deprecated_compiled_contract_class = state_reader
Here, consider adding a comment for cairo0 classes
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3568 at r3 (raw file):
} #[tokio::test]
Add todo to the test to add cases for block numbers that are not the latest.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3570 at r3 (raw file):
#[tokio::test] async fn get_compiled_contract_class() { let casm_class_hash = ClassHash(felt!("0x1"));
Consider using Felt constants. Felt::ONE...
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3570 at r3 (raw file):
#[tokio::test] async fn get_compiled_contract_class() { let casm_class_hash = ClassHash(felt!("0x1"));
Consider changing this to sierra_class_hash
. The compiled class hash is a different thing.
Code quote:
casm_class_hash
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3586 at r3 (raw file):
.append_state_diff( BlockNumber(0), starknet_api::state::ThinStateDiff {
You forgot the field deprecated_declared_classes
of the ThinStateDiff
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3594 at r3 (raw file):
.append_casm(&casm_class_hash, &casm_contract_class) .unwrap() .append_classes(BlockNumber(0), &[], &[(deprecated_class_hash, &deprecated_contract_class)])
Please also add Sierra corresponding to the casm to the storage. Or at least add a comment about that
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3614 at r3 (raw file):
assert_eq!(res, CompiledContractClass::V0(deprecated_contract_class)); // Ask for an invalid class hash.
consider: not existing
Code quote:
an invalid
8dfd3ed
to
853016b
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.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1444 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Do it in one line
but I need both storage_txn and state_reader for the following code.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1446 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Consider adding a comment here to say that this is the logic for cairo1
done.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1461 at r3 (raw file):
Previously, DvirYo-starkware wrote…
This error should be
internal_server_error
. The fact we fail to get the state after the block has nothing to do with the CLASS_HASH_NOT_FOUND error.
done.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1462 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Here, consider adding a comment for cairo0 classes
done
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3568 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Add todo to the test to add cases for block numbers that are not the latest.
done.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3570 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Consider using Felt constants. Felt::ONE...
done
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3570 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Consider changing this to
sierra_class_hash
. The compiled class hash is a different thing.
right.
changed everything to cairo1 and cairo0 for readability.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3586 at r3 (raw file):
Previously, DvirYo-starkware wrote…
You forgot the field
deprecated_declared_classes
of theThinStateDiff
I did not forget it, I just went through the code and saw that this field is not used in this flow.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3594 at r3 (raw file):
Previously, DvirYo-starkware wrote…
Please also add Sierra corresponding to the casm to the storage. Or at least add a comment about that
done.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3614 at r3 (raw file):
Previously, DvirYo-starkware wrote…
consider: not existing
Done.
853016b
to
34ff089
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.
Reviewable status: 1 of 3 files reviewed, 5 unresolved discussions (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3586 at r3 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
I did not forget it, I just went through the code and saw that this field is not used in this flow.
Done.
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 r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1444 at r3 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
but I need both storage_txn and state_reader for the following code.
You right.
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1446 at r5 (raw file):
let state_reader = storage_txn.get_state_reader().map_err(internal_server_error)?; let block_number = get_accepted_block_number(&storage_txn, block_id)?; // Check if this class exists in the Cairo1 classes table.
Consider adding a new line here
Code quote:
let block_number = get_accepted_block_number(&storage_txn, block_id)?;
// Check if this class exists in the Cairo1 classes table.
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3569 at r5 (raw file):
#[tokio::test] // TODO (Yael 16/06/2024): Add a test case for block_number which is not the latest.
move it before the #[tokio::test]
Code quote:
// TODO (Yael 16/06/2024): Add a test case for block_number which is not the latest.
34ff089
to
a39a408
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.
Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @dan-starkware and @DvirYo-starkware)
crates/papyrus_rpc/src/v0_7/api/api_impl.rs
line 1446 at r5 (raw file):
Previously, DvirYo-starkware wrote…
Consider adding a new line here
done
crates/papyrus_rpc/src/v0_7/api/test.rs
line 3569 at r5 (raw file):
Previously, DvirYo-starkware wrote…
move it before the
#[tokio::test]
done
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 r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
#2093) feat(JSON-RPC): add deprecated contract class to getCompiledContractClass
…lass
This change is