feat(JSON-RPC): add deprecated contract class to getCompiledContractC…

[Yael-Starkware]

…lass

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.30%. Comparing base (14ee3bb) to head (a39a408).
Report is 9 commits behind head on main.

Files	Patch %	Lines
crates/papyrus_rpc/src/v0_7/api/api_impl.rs	0.00%	1 Missing ⚠️

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.
📢 Have feedback on the report? Share it here.

[DvirYo-starkware]

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

[Yael-Starkware]

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.

[Yael-Starkware]

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?

[Yael-Starkware]

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

[Yael-Starkware]

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.

[DvirYo-starkware]

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

[Yael-Starkware]

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 the ThinStateDiff

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.

[Yael-Starkware]

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.

[DvirYo-starkware]

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.

[Yael-Starkware]

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

[DvirYo-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)