refactor: rpc types #395

jbcaron · 2024-11-22T17:11:16Z

Pull Request type

Please add the labels corresponding to the type of changes your PR introduces:

Refactoring (no functional changes, no API changes)

What is the current behavior?

Resolves: #382

What is the new behavior?

Does this introduce a breaking change?

Yes DB

Other information

Trantorian1

Awesome work, seems like this will be much more pleasant to develop with in the future :)

Trantorian1 · 2024-11-28T07:22:23Z

Cargo.lock

 [[package]]
 name = "starknet-types-rpc"
 version = "0.7.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2d717a1b5ff67fa9ec13b09381513a4824fdd73ec5440fcc116088be6744aac4"
+source = "git+https://github.com/jbcaron/types-rs.git?branch=fork#234b5f03f91d7f791003e61ae1cfcfc1f096a539"


Maybe we should move this to madara-alliance?

Trantorian1 · 2024-11-28T07:22:41Z

Cargo.lock

@@ -8643,7 +8651,7 @@ dependencies = [
 "serde_json",
 "sha3",
 "starknet-crypto 0.5.2",
- "starknet-types-core",
+ "starknet-types-core 0.1.7 (git+https://github.com/kasarlabs/types-rs.git?branch=feat-deserialize-v0.1.7)",


Same for this repo

Trantorian1 · 2024-11-28T07:36:11Z

crates/client/exec/src/trace.rs

+    let execution_resources = starknet_types_rpc::ExecutionResources {
+        bitwise_builtin_applications: computation_resources.bitwise_builtin_applications,
+        ec_op_builtin_applications: computation_resources.ec_op_builtin_applications,
+        ecdsa_builtin_applications: computation_resources.ecdsa_builtin_applications,
+        keccak_builtin_applications: computation_resources.keccak_builtin_applications,
+        memory_holes: computation_resources.memory_holes,
+        pedersen_builtin_applications: computation_resources.pedersen_builtin_applications,
+        poseidon_builtin_applications: computation_resources.poseidon_builtin_applications,
+        range_check_builtin_applications: computation_resources.range_check_builtin_applications,
+        segment_arena_builtin: computation_resources.segment_arena_builtin,
+        steps: computation_resources.steps,
+        data_availability: starknet_types_rpc::DataAvailability {
+            l1_gas: execution_info.transaction_receipt.da_gas.l1_gas,
+            l1_data_gas: execution_info.transaction_receipt.da_gas.l1_data_gas,


Since we are working off a fork, consider implementing From

crates/client/exec/src/trace.rs

crates/client/rpc/src/test_utils.rs

crates/primitives/receipt/src/into_starknet_types.rs

Trantorian1 · 2024-11-28T08:29:33Z

crates/primitives/receipt/src/into_starknet_types.rs

+// impl From<starknet_types_rpc::DataAvailability> for L1Gas {
+//     fn from(resources: starknet_types_rpc::DataAvailability) -> Self {
+//         Self { l1_gas: resources.l1_gas.into(), l1_data_gas: resources.l1_data_gas.into() }
+//     }
+// }
+


Suggested change

// impl From<starknet_types_rpc::DataAvailability> for L1Gas {

// fn from(resources: starknet_types_rpc::DataAvailability) -> Self {

// Self { l1_gas: resources.l1_gas.into(), l1_data_gas: resources.l1_data_gas.into() }

// }

// }

crates/primitives/receipt/src/into_starknet_types.rs

Trantorian1 · 2024-11-28T08:30:49Z

crates/primitives/receipt/src/into_starknet_types.rs

+// #[cfg(test)]
+// mod test {
+//     use super::*;
+//     use crate::{
+//         tests::dummy_declare_receipt, tests::dummy_deploy_account_receipt, tests::dummy_deploy_receipt,
+//         tests::dummy_invoke_receipt, tests::dummy_l1_handler_receipt,
+//     };
+
+//     #[test]
+//     fn test_into_starknet_types_receipt() {
+//         let receipt: TransactionReceipt = dummy_invoke_receipt().into();
+//         let core_receipt = receipt.clone().to_starknet_types(starknet_types_rpc::TxnFinalityStatus::L1);
+//         let receipt_back: TransactionReceipt = core_receipt.into();
+//         assert_eq!(receipt, receipt_back);
+
+//         let receipt: TransactionReceipt = dummy_l1_handler_receipt().into();
+//         let core_receipt = receipt.clone().to_starknet_types(starknet_types_rpc::TxnFinalityStatus::L1);
+//         let receipt_back: TransactionReceipt = core_receipt.into();
+//         assert_eq!(receipt, receipt_back);
+
+//         let receipt: TransactionReceipt = dummy_declare_receipt().into();
+//         let core_receipt = receipt.clone().to_starknet_types(starknet_types_rpc::TxnFinalityStatus::L1);
+//         let receipt_back: TransactionReceipt = core_receipt.into();
+//         assert_eq!(receipt, receipt_back);
+
+//         let receipt: TransactionReceipt = dummy_deploy_receipt().into();
+//         let core_receipt = receipt.clone().to_starknet_types(starknet_types_rpc::TxnFinalityStatus::L1);
+//         let receipt_back: TransactionReceipt = core_receipt.into();
+//         assert_eq!(receipt, receipt_back);
+
+//         let receipt: TransactionReceipt = dummy_deploy_account_receipt().into();
+//         let core_receipt = receipt.clone().to_starknet_types(starknet_types_rpc::TxnFinalityStatus::L1);
+//         let receipt_back: TransactionReceipt = core_receipt.into();
+//         assert_eq!(receipt, receipt_back);
+//     }
+// }


Why remove these tests?

Trantorian1 · 2024-11-28T08:33:12Z

crates/primitives/transactions/src/broadcasted_to_blockifier.rs

-                let class_info =
-                    SierraClassInfo { contract_class: Arc::new(flatten_sierra_class), compiled_class_hash };
+                        (
+                            Some(blockifier::execution::contract_class::ClassInfo::new(&class_blockifier, 0, 0)?),
+                            Some(class_hash),
+                            Some(ConvertedClass::Legacy(LegacyConvertedClass { class_hash, info: class_info })),
+                        )
+                    }
+                    starknet_types_rpc::BroadcastedDeclareTxn::V2(starknet_types_rpc::BroadcastedDeclareTxnV2 {
+                        compiled_class_hash,
+                        contract_class,
+                        ..
+                    })
+                    | starknet_types_rpc::BroadcastedDeclareTxn::QueryV2(
+                        starknet_types_rpc::BroadcastedDeclareTxnV2 { compiled_class_hash, contract_class, .. },
+                    )
+                    | starknet_types_rpc::BroadcastedDeclareTxn::V3(starknet_types_rpc::BroadcastedDeclareTxnV3 {
+                        compiled_class_hash,
+                        contract_class,
+                        ..
+                    })
+                    | starknet_types_rpc::BroadcastedDeclareTxn::QueryV3(
+                        starknet_types_rpc::BroadcastedDeclareTxnV3 { compiled_class_hash, contract_class, .. },
+                    ) => {
+                        let flatten_sierra_class: FlattenedSierraClass = contract_class.clone().into();
+                        let class_hash = flatten_sierra_class
+                            .compute_class_hash()
+                            .map_err(BroadcastedToBlockifierError::ComputeSierraClassHashFailed)?;
+                        let (compiled_class_hash_computed, compiled) = flatten_sierra_class.compile_to_casm()?;
+                        if compiled_class_hash != &compiled_class_hash_computed {
+                            return Err(BroadcastedToBlockifierError::CompiledClassHashMismatch {
+                                expected: *compiled_class_hash,
+                                compilation: compiled_class_hash_computed,
+                            });
+                        }
+                        let class_info = SierraClassInfo {
+                            contract_class: Arc::new(flatten_sierra_class),
+                            compiled_class_hash: compiled_class_hash_computed,
+                        };

-                (
-                    Some(blockifier::execution::contract_class::ClassInfo::new(
-                        &compiled.to_blockifier_class()?,
-                        tx.contract_class.sierra_program.len(),
-                        tx.contract_class.abi.len(),
-                    )?),
-                    Some(class_hash),
-                    Some(ConvertedClass::Sierra(SierraConvertedClass {
-                        class_hash,
-                        info: class_info,
-                        compiled: Arc::new(compiled),
-                    })),
-                )
-            }
-            starknet_core::types::BroadcastedDeclareTransaction::V3(tx) => {
-                let class_hash = tx.contract_class.class_hash();
-                let flatten_sierra_class: FlattenedSierraClass = (*tx.contract_class).clone().into();
-                let (compiled_class_hash, compiled) = flatten_sierra_class.compile_to_casm()?;
-                if tx.compiled_class_hash != compiled_class_hash {
-                    return Err(BroadcastedToBlockifierError::CompiledClassHashMismatch {
-                        expected: tx.compiled_class_hash,
-                        compilation: compiled_class_hash,
-                    });
+                        (
+                            Some(blockifier::execution::contract_class::ClassInfo::new(
+                                &compiled.to_blockifier_class()?,
+                                contract_class.sierra_program.len(),
+                                contract_class.abi.as_ref().map(|abi| abi.len()).unwrap_or(0),
+                            )?),
+                            Some(class_hash),
+                            Some(ConvertedClass::Sierra(SierraConvertedClass {
+                                class_hash,
+                                info: class_info,
+                                compiled: Arc::new(compiled),
+                            })),
+                        )
+                    }
                }
-                let class_info =
-                    SierraClassInfo { contract_class: Arc::new(flatten_sierra_class), compiled_class_hash };
-
-                (
-                    Some(blockifier::execution::contract_class::ClassInfo::new(
-                        &compiled.to_blockifier_class()?,
-                        tx.contract_class.sierra_program.len(),
-                        tx.contract_class.abi.len(),
-                    )?),
-                    Some(class_hash),
-                    Some(ConvertedClass::Sierra(SierraConvertedClass {
-                        class_hash,
-                        info: class_info,
-                        compiled: Arc::new(compiled),
-                    })),
-                )
            }
-        },
-        _ => (None, None, None),
-    };
+            _ => (None, None, None),
+        };


Consider refactoring this into sub-functions to make it more readable, I'm havin a hard time sifting through this on review 😵‍💫

jbcaron self-assigned this Nov 22, 2024

jbcaron force-pushed the refactor/rpc-types branch 3 times, most recently from 62c7fa6 to 8d3805b Compare November 27, 2024 18:43

jbcaron marked this pull request as ready for review November 27, 2024 18:44

Trantorian1 reviewed Nov 28, 2024

View reviewed changes

jbcaron force-pushed the refactor/rpc-types branch 2 times, most recently from 6ce4c39 to ec63b05 Compare November 29, 2024 13:00

jbcaron added 5 commits November 29, 2024 14:11

fix(fgw): include l1_to_l2_consumed_message in L1 handler receipt

d1d6505

add add_transaction gateway client

c7baf4c

fix comment, remove crate error

6043450

refactor: migrate to starknet_type_rpc

daf8b21

fix: comment

82e599d

jbcaron force-pushed the refactor/rpc-types branch 2 times, most recently from b20a6b2 to e8c7339 Compare November 29, 2024 13:29

fix: ci

c472425

jbcaron force-pushed the refactor/rpc-types branch from e8c7339 to c472425 Compare November 29, 2024 14:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: rpc types #395

refactor: rpc types #395

jbcaron commented Nov 22, 2024 •

edited

Loading

Trantorian1 left a comment

Trantorian1 Nov 28, 2024

Trantorian1 Nov 28, 2024

Trantorian1 Nov 28, 2024

Trantorian1 Nov 28, 2024

Trantorian1 Nov 28, 2024

Trantorian1 Nov 28, 2024

refactor: rpc types #395

Are you sure you want to change the base?

refactor: rpc types #395

Conversation

jbcaron commented Nov 22, 2024 • edited Loading

Pull Request type

What is the current behavior?

What is the new behavior?

Does this introduce a breaking change?

Other information

Trantorian1 left a comment

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

Trantorian1 Nov 28, 2024

Choose a reason for hiding this comment

jbcaron commented Nov 22, 2024 •

edited

Loading