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 #187

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: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 😵‍💫

Trantorian1

Seems great!

Trantorian1 · 2024-12-02T07:57:19Z

crates/primitives/receipt/src/to_starknet_types.rs

+    }
+}
+
+fn nullify_zero(u: u64) -> Option<u64> {


Consider making this a helper function inside of impl From<ExecutionResources> for starknet_types_rpc::ExecutionResources (nitpick)

antiyro

tested locally works fine on e2e test read calls

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 7 times, most recently from ca4977e to 48aa647 Compare December 1, 2024 19:26

Trantorian1 approved these changes Dec 2, 2024

View reviewed changes

jbcaron requested a review from antiyro December 2, 2024 13:15

jbcaron added 5 commits December 2, 2024 14:16

fix(fgw): include l1_to_l2_consumed_message in L1 handler receipt

050f2e3

add add_transaction gateway client

a7247c2

fix comment, remove crate error

33870f8

refactor: migrate to starknet_type_rpc

860d099

fix: comment

c40178c

jbcaron force-pushed the refactor/rpc-types branch from 48aa647 to 64ebb10 Compare December 2, 2024 22:03

fix: ci

2b95231

jbcaron force-pushed the refactor/rpc-types branch from 64ebb10 to 2b95231 Compare December 3, 2024 15:34

antiyro approved these changes Dec 4, 2024

View reviewed changes

antiyro merged commit 86b2380 into main Dec 4, 2024
10 checks passed

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 left a comment

Trantorian1 Dec 2, 2024

antiyro left a comment

refactor: rpc types #395

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 left a comment

Choose a reason for hiding this comment

Trantorian1 Dec 2, 2024

Choose a reason for hiding this comment

antiyro left a comment

Choose a reason for hiding this comment

jbcaron commented Nov 22, 2024 •

edited

Loading