V8 get storage proof tests #272
Conversation
WalkthroughThis pull request introduces several new modules and functionalities. In the contracts directory, three new modules (smpl20, smpl21, and smpl22) are appended to the library file, each containing a HelloStarknet contract implementation that includes balance management, deposit handling from Layer 1, and event logging. In the openrpc-testgen suite, new test modules and TestCase implementations are added to validate storage proofs for classes, contracts, and contract storage. Additionally, the JSON-RPC provider and utility modules are enhanced with new error handling and storage proof retrieval methods, along with a dedicated module for Merkle tree types and proof verification. Changes
Sequence Diagram(s)sequenceDiagram
participant L1
participant HelloStarknet
participant Storage
L1->>HelloStarknet: deposit(from_address, user, amount)
HelloStarknet->>Storage: Retrieve current balance for user
Storage-->>HelloStarknet: Return current balance
HelloStarknet->>Storage: Update balance with deposited amount
HelloStarknet->>L1: Emit DepositFromL1 event
sequenceDiagram
participant Client
participant JsonRpcClient
participant Provider
participant Node
participant MerkleTree
Client->>JsonRpcClient: Request get_storage_proof(block_id, params)
JsonRpcClient->>Provider: Send GetStorageProof request
Provider->>Node: Query storage proof data
Node-->>Provider: Return proof data
Provider-->>JsonRpcClient: Provide GetStorageProofResult
JsonRpcClient->>MerkleTree: Verify proof via Merkle tree methods
MerkleTree-->>JsonRpcClient: Return verification result
JsonRpcClient->>Client: Return final storage proof outcome
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
contracts/src/lib.cairo(1 hunks)contracts/src/smpl20.cairo(1 hunks)contracts/src/smpl21.cairo(1 hunks)contracts/src/smpl22.cairo(1 hunks)openrpc-testgen/src/suite_openrpc/mod.rs(1 hunks)openrpc-testgen/src/suite_openrpc/suite_deploy/suite_contract_calls/test_get_storage_at.rs(1 hunks)openrpc-testgen/src/suite_openrpc/test_get_storage_class_proof.rs(1 hunks)openrpc-testgen/src/suite_openrpc/test_get_storage_contract_proof.rs(1 hunks)openrpc-testgen/src/suite_openrpc/test_get_storage_contract_storage_proof.rs(1 hunks)openrpc-testgen/src/utils/mod.rs(1 hunks)openrpc-testgen/src/utils/v7/endpoints/errors.rs(2 hunks)openrpc-testgen/src/utils/v7/providers/jsonrpc/mod.rs(5 hunks)openrpc-testgen/src/utils/v7/providers/provider.rs(2 hunks)openrpc-testgen/src/utils/v8/mod.rs(1 hunks)openrpc-testgen/src/utils/v8/types.rs(1 hunks)
🔇 Additional comments (49)
contracts/src/smpl22.cairo (1)
1-5: Interface definition looks good.
This trait is concise and clearly defines the two core functions for balance management.contracts/src/smpl20.cairo (2)
1-5: Interface definition is consistent.
The interface matches other modules, ensuring standardization across the codebase.
32-37: Clarifyfrom_addressusage or remove it if unnecessary.
Sincefrom_addressis not referenced, confirm if you need it to verify deposit origin or remove it to reduce confusion.contracts/src/smpl21.cairo (2)
1-5: Good reusability of the interface.
The interface remains consistent with the other modules (smpl20, smpl22), which is helpful for maintainability.
39-48: Evaluate dual-balance pattern.
increase_balancesets a global balance whereasdepositupdates user balances. Make sure it aligns with your intended contract logic.openrpc-testgen/src/suite_openrpc/test_get_storage_class_proof.rs (5)
1-19: No issues found in the imports
21-22: Struct definition
No issues found.
24-27: Trait implementation block
This trait-based approach neatly modularizes test logic, making the suite easier to extend and maintain.
51-60: Confirm block usage
Retrieving the proof at the latest block may lead to non-deterministic results if the chain is active. When stable results are needed, specify an exact block number to avoid potential race conditions.
62-71: Additional proof validations
Verifying the class proof using the Poseidon hash is correct. Consider validating all available fields in the proof (e.g., sibling paths, additional leaves) to ensure the entire proof is sound.openrpc-testgen/src/suite_openrpc/test_get_storage_contract_proof.rs (7)
1-19: No issues found in the imports
27-28: Struct definition
No issues found.
30-33: Trait implementation block
Good pattern for organizing test cases.
76-104: Receipt type handling
Using a match to handle different receipt types is a clean and explicit way to manage various transaction outcomes.
111-130: Check block choice
UsingBlockTag::Latestcan introduce non-determinism if new blocks arrive during test execution. When reproducibility is needed, a specific block number might be preferable.
132-146: Class hash and nonce checks
Verifying both nonce and class hash against expected values is a solid step for integrity.
148-156: Proof verification
UsingMerkleTreeMadarawith Pedersen hashing is appropriate for proof validation. Good job ensuring the proof matches the computed child node.openrpc-testgen/src/suite_openrpc/test_get_storage_contract_storage_proof.rs (7)
1-19: No issues found in the imports
32-33: Struct definition
No concerns; straightforward data holder for the test.
35-38: Trait boilerplate
ImplementingRunnableTraitkeeps the test approach consistent with other modules.
81-108: Receipt type resolution
The match arms properly differentiate possible receipt types, with explicit error handling for unexpected scenarios.
111-124: Check block choice
If the chain is subject to updates, retrieving proofs atBlockTag::Latestmay introduce timing issues. Specifying an exact block number is safer for deterministic end-to-end tests.
125-139: Initial storage proof check
Ensuring that the initial storage proof is empty is a good correctness check for newly deployed contracts.
185-190: Final proof verification
Correct usage of the Merkle tree approach, verifying the updated storage is reflected accurately in the proof.openrpc-testgen/src/utils/v8/types.rs (17)
1-10: Imports look good
All required crates appear properly included, ensuring needed functionality (serialization, hashing, errors).
19-24: Looks good
The struct is straightforward and aligns well with storage key usage.
33-37: Well-defined structure
Thenode_hashandnodefields have clear purposes.
39-43: Straightforward Merkle tree holder
Storing a node map and the root hash is a clear approach for reconstructing and traversing the Merkle tree.
52-64: Enum structure is clean
Binary vs. Edge clearly captures distinct Merkle node types.
66-70: Verify no overlapping definitions
Ensure thatContractsProofdoes not replicate data structures already stored in other proof types.
72-77: Stable definition
Simple fields forContractLeavesDataItem; no immediate concerns.
78-83: Global roots naming
contracts_tree_root,classes_tree_root,block_hashare suitably descriptive.
85-85: Hash function type alias
Providing a flexible alias invites easily swapping different hash functions.
87-87: Check short_string stability
Verify thatshort_string!("CONTRACT_CLASS_LEAF_V0")consistently meets protocol requirements for class leaves.
89-121: Comprehensive error handling
ProofErrorcovers a broad range of mismatch and missing-data scenarios. Good thoroughness.
123-185: Efficient from_proof construction
The logic correctly rebuilds node relationships. Consider edge cases where multiple parents might share the same child to ensure no collisions.
211-238: Edge hashing approach
Addinglengthto the result is correct per your spec, but double-check for largelengthvalues that could cause potential overflows.
302-305: Confirm correct hashing
Uses Poseidon for class-proof computation. Ensure this aligns with the protocol’s requirements.
307-319: Chain of Pedersen hashes
This approach for contract proofs appears correct. Confirm the zero usage is consistent with official specs.
325-403: Positive test coverage
test_valid_proofthoroughly checks normal conditions. Consider adding boundary-edge scenarios.
405-483: Negative test coverage
test_invalid_proofensures error handling is triggered as expected. Good job catching mismatch cases.openrpc-testgen/src/utils/v7/endpoints/errors.rs (4)
14-14: ImportingProofError
Neatly unifies proof-related errors underOpenRpcTestGenError.
18-18:FromStrErrorreintroduction
Restores parsing error coverage for user or config-based inputs.
20-20:thiserror::Errorimport
Crucial for consistently deriving error implementations.
96-97: Unified proof error variant
Proof(#[from] ProofError)elegantly composes proof errors into the main enum.openrpc-testgen/src/suite_openrpc/mod.rs (1)
66-68: Added storage proof test modules
These modules broaden test coverage for class/contract storage proofs. Ensure they're run in CI for full coverage.openrpc-testgen/src/utils/v7/providers/jsonrpc/mod.rs (1)
49-50: LGTM!The implementation is well-structured and consistent with the codebase patterns:
- Proper error handling through the Result type
- Consistent with other RPC method implementations
- Clean integration with the serialization/deserialization system
Also applies to: 113-113, 290-307
openrpc-testgen/src/utils/v8/mod.rs (1)
1-1: LGTM!The module organization is clean and follows Rust best practices.
openrpc-testgen/src/utils/mod.rs (1)
5-5: LGTM!The addition of the v8 module follows the established version-based module organization pattern.
| balance: felt252, | ||
| balances: Map<felt252, felt252>, | ||
| another_arg13: felt252, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing or documenting unused field another_arg13.
This field doesn't appear to be used. If it's intentional for future use, add a comment to clarify its purpose; otherwise, remove it to reduce clutter.
| #[l1_handler] | ||
| fn deposit(ref self: ContractState, from_address: felt252, user: felt252, amount: felt252) { | ||
| let balance = self.balances.read(user); | ||
| self.balances.write(user, balance + amount); | ||
| self.emit(DepositFromL1 { user, amount }); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unused parameter from_address.
Currently, from_address is not used in the deposit function. If you don’t need it, consider removing it to avoid confusion; otherwise, clarify its purpose.
| #[abi(embed_v0)] | ||
| impl HelloStarknetImpl of super::IHelloStarknet<ContractState> { | ||
| fn increase_balance(ref self: ContractState, amount: felt252) { | ||
| self.balance.write(self.balance.read() + amount); | ||
| } | ||
|
|
||
| fn get_balance(self: @ContractState) -> felt252 { | ||
| self.balance.read() | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align global vs. user balance management.
increase_balance affects the global balance field, whereas deposit updates user-specific balances. If this design is intentional (e.g., you maintain both a global tally and per-user balances), clarify it. Otherwise, consider consolidating to avoid confusion or accidental discrepancies.
| balance: felt252, | ||
| balances: Map<felt252, felt252>, | ||
| another_arg11: felt252, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Document the purpose of another_arg11.
If another_arg11 is not currently used, consider removing it or documenting its intended use.
| #[abi(embed_v0)] | ||
| impl HelloStarknetImpl of super::IHelloStarknet<ContractState> { | ||
| fn increase_balance(ref self: ContractState, amount: felt252) { | ||
| self.balance.write(self.balance.read() + amount); | ||
| } | ||
|
|
||
| fn get_balance(self: @ContractState) -> felt252 { | ||
| self.balance.read() | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Ensure consistency between global and user-specific balances.
As with smpl22, you have a global balance in addition to balances. Ensure this dual-tracking is intentional and thoroughly tested to avoid mismatch or bugs.
| #[derive(Debug, Clone)] | ||
| pub struct MadaraTreeNode { | ||
| pub node: MerkleNode, | ||
| pub node_hash: Felt, | ||
| pub parent_hash: Option<Felt>, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify the dual storage of node & node_hash
Consider adding doc comments explaining why both node (with child data) and node_hash are separately stored.
| /// Finds and returns a reference to the `MadaraTreeNode` containing an edge node | ||
| /// whose child field matches the given `expected_child`. | ||
| /// | ||
| /// This function iterates over all nodes in the Merkle tree and checks if they | ||
| /// are edge nodes with a child field equal to `expected_child`. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `expected_child` - A reference to a `Felt` representing the expected child | ||
| /// hash of the edge node. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// An `Option` containing a reference to the matching `MadaraTreeNode` if found, | ||
| /// or `None` if no such node exists. | ||
| pub fn find_matching_edge_node(&self, expected_child: &Felt) -> Option<&MadaraTreeNode> { | ||
| self.nodes.values().find(|node| { | ||
| if let MerkleNode::Edge { child, .. } = &node.node { | ||
| child == expected_child | ||
| } else { | ||
| false | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Edge node search
This linear lookup is fine for small proofs. For larger sets, consider indexing by child hash for efficiency.
| /// Verifies the storatge proof in the Merkle tree. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `expected_child` - The expected child child value of edge node | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// `Ok(true)` if the proof is valid, or `Err(ProofError)` if the proof is invalid. | ||
| /// | ||
| /// The verification process works as follows: | ||
| /// | ||
| /// 1. Find the edge node in the Merkle tree with the given expected child hash. | ||
| /// 2. Compute the hash of the edge node. | ||
| /// 3. Iterate over the parents of the edge node until the root of the Merkle tree | ||
| /// is reached. For each parent, check that the child field matches the current | ||
| /// hash, and if so, compute the hash of the parent. | ||
| /// 4. If the final computed root matches the root of the Merkle tree, return | ||
| /// `true`, otherwise return an error. | ||
| pub fn verify_proof(&self, expected_child: &Felt, hash_fn: HashFn) -> Result<bool, ProofError> { | ||
| let edge_node = self | ||
| .find_matching_edge_node(expected_child) | ||
| .ok_or_else(|| ProofError::NoMatchingEdgeNode(*expected_child))?; | ||
| let mut current_node = edge_node; | ||
| let mut current_hash = self.compute_node_hash(current_node, hash_fn)?; | ||
|
|
||
| while let Some(parent_hash) = current_node.parent_hash { | ||
| let parent = self | ||
| .nodes | ||
| .get(&parent_hash) | ||
| .ok_or_else(|| ProofError::NoParentFound(parent_hash))?; | ||
|
|
||
| // Check if the computed hash is equal to any of the parent's children, or if it matches the parent's child field | ||
| match &parent.node { | ||
| MerkleNode::Binary { left, right } => { | ||
| if *left != current_hash && *right != current_hash { | ||
| return Err(ProofError::ChildMismatch { | ||
| computed: current_hash, | ||
| left: *left, | ||
| right: *right, | ||
| node_hash: parent.node_hash, | ||
| }); | ||
| } | ||
| } | ||
| MerkleNode::Edge { child, .. } => { | ||
| if *child != current_hash { | ||
| return Err(ProofError::EdgeChildMismatch { | ||
| computed: current_hash, | ||
| child: *child, | ||
| node_hash: parent.node_hash, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| current_hash = self.compute_node_hash(parent, hash_fn)?; | ||
| current_node = parent; | ||
| } | ||
|
|
||
| Ok(current_hash == self.root) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Doc comment typo & robust logic
“storatge” → “storage” in the comment. Overall, the step-by-step verification flow seems thorough.
| fn get_storage_proof( | ||
| &self, | ||
| block_id: BlockId<Felt>, | ||
| class_hashes: Option<Vec<Felt>>, | ||
| contract_addresses: Option<Vec<Felt>>, | ||
| contracts_storage_keys: Option<Vec<ContractStorageKeysItem>>, | ||
| ) -> impl std::future::Future<Output = Result<GetStorageProofResult, ProviderError>>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Add documentation for the new method.
The get_storage_proof method is missing documentation. Consider adding a doc comment explaining:
- The purpose of the method
- The parameters and their significance
- The expected return value
- Any potential error cases
Apply this diff to add documentation:
+ /// Get the storage proof for specified contract addresses and storage keys
+ ///
+ /// # Arguments
+ /// * `block_id` - The block identifier
+ /// * `class_hashes` - Optional list of class hashes to get proofs for
+ /// * `contract_addresses` - Optional list of contract addresses to get proofs for
+ /// * `contracts_storage_keys` - Optional list of contract storage keys to get proofs for
+ ///
+ /// # Returns
+ /// Storage proof result containing Merkle proofs for the requested items
fn get_storage_proof(
&self,
block_id: BlockId<Felt>,
class_hashes: Option<Vec<Felt>>,
contract_addresses: Option<Vec<Felt>>,
contracts_storage_keys: Option<Vec<ContractStorageKeysItem>>,
) -> impl std::future::Future<Output = Result<GetStorageProofResult, ProviderError>>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn get_storage_proof( | |
| &self, | |
| block_id: BlockId<Felt>, | |
| class_hashes: Option<Vec<Felt>>, | |
| contract_addresses: Option<Vec<Felt>>, | |
| contracts_storage_keys: Option<Vec<ContractStorageKeysItem>>, | |
| ) -> impl std::future::Future<Output = Result<GetStorageProofResult, ProviderError>>; | |
| /// Get the storage proof for specified contract addresses and storage keys | |
| /// | |
| /// # Arguments | |
| /// * `block_id` - The block identifier | |
| /// * `class_hashes` - Optional list of class hashes to get proofs for | |
| /// * `contract_addresses` - Optional list of contract addresses to get proofs for | |
| /// * `contracts_storage_keys` - Optional list of contract storage keys to get proofs for | |
| /// | |
| /// # Returns | |
| /// Storage proof result containing Merkle proofs for the requested items | |
| fn get_storage_proof( | |
| &self, | |
| block_id: BlockId<Felt>, | |
| class_hashes: Option<Vec<Felt>>, | |
| contract_addresses: Option<Vec<Felt>>, | |
| contracts_storage_keys: Option<Vec<ContractStorageKeysItem>>, | |
| ) -> impl std::future::Future<Output = Result<GetStorageProofResult, ProviderError>>; |
| println!( | ||
| "deployed_contract_address {:#?}", | ||
| test_input.deployed_contract_address, | ||
| ); | ||
|
|
||
| println!("contract_balance_slot {:#?}", contract_balance_slot); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Logging for Debugging: Consider Using a Configurable Logging Framework
The newly added println! statements at the end of the run method output the deployed contract address and storage slot, which can be very useful for debugging purposes. For long-term maintainability and to better control the verbosity (especially in CI or production-like test environments), consider switching to a logging framework (for example, using the log crate with an appropriate logger like env_logger or tracing). This approach would allow you to conditionally enable or disable detailed output based on configuration rather than having these printouts always active.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
openrpc-testgen/src/suite_openrpc/test_get_storage_class_proof.rs(1 hunks)openrpc-testgen/src/suite_openrpc/test_get_storage_contract_proof.rs(1 hunks)openrpc-testgen/src/suite_openrpc/test_get_storage_contract_storage_proof.rs(1 hunks)openrpc-testgen/src/utils/v8/types.rs(1 hunks)
🔇 Additional comments (13)
openrpc-testgen/src/suite_openrpc/test_get_storage_class_proof.rs (2)
28-36: Use external configuration for file paths
This logic replicates earlier concerns about hard-coded paths. Centralizing file path management can greatly enhance maintainability, e.g., using a config file or environment variables.
1-75: Overall implementation looks solid
Contract declaration, transaction waiting, storage proof retrieval, and Merkle tree verification appear consistent. Good job!openrpc-testgen/src/suite_openrpc/test_get_storage_contract_proof.rs (2)
34-42: Externalize contract file paths
Similar to prior feedback, consider introducing a config-based approach to handle file paths for better portability and versioning.
58-61: Salt generation recheck
Past reviews mention the trade-offs of random salt for each test run. If deterministic runs are desired, consider using a fixed or seeded salt approach.openrpc-testgen/src/suite_openrpc/test_get_storage_contract_storage_proof.rs (2)
39-46: Externalize contract file paths
Reiterating earlier feedback: hard-coded paths become upkeep bottlenecks. A centralized config or environment variable strategy often scales better.
63-66: Salt generation consistency
As previously noted, randomized salts affect reproducibility. Use a fixed seed or alternative approach if test determinism is a priority.openrpc-testgen/src/utils/v8/types.rs (7)
12-17: Consider renaming the generic parameterFto a more descriptive term
This helps improve clarity by indicating that it may be a field element or similar numeric type.
28-29: Unify the naming ofcontracts_proofandcontracts_storage_proofs
Consistent naming across fields would improve readability and maintainability.
34-37: Clarify the dual storage ofnodeandnode_hash
Adding doc comments explaining why both are necessary (e.g., references to parent-child hashing logic) would help future maintainers.
202-209: Edge node search optimization
A linear lookup is acceptable for small proofs, but consider indexing by child hash for larger sets.
232-232: Verify the addition oflengthto the hash
Confirm that appendingFelt::from(*length)is the intended behavior for encoding edge-path length in the hash.
236-236: Fix typo in doc comment
Change “storatge” to “storage” to ensure clarity and consistency in documentation.
298-315: Use of Pedersen and Poseidon hashing is aligned with StarkNet standards
The CPU-friendly Poseidon and collision-resistant Pedersen are appropriate for these proofs. This approach looks solid.
| let expected_child = | ||
| merkle_tree.compute_expected_child_for_class_proof(&compiled_class_hash); | ||
| let valid_proof = merkle_tree.verify_proof(&expected_child, Poseidon::hash)?; | ||
| assert_result!(valid_proof, "Class proof verification failed"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Unify hashing approach or verify correctness
You’re using Poseidon here, whereas related proof code in other files uses Pedersen. Verify that both hashing approaches are intentionally distinct or consider standardizing the hash function across tests for easier maintenance.
| let declare_result = sender | ||
| .declare_v3(flattened_sierra_class, compiled_class_hash) | ||
| .send() | ||
| .await?; | ||
|
|
||
| wait_for_sent_transaction( | ||
| declare_result.transaction_hash, | ||
| &test_input.random_paymaster_account.random_accounts()?, | ||
| ) | ||
| .await?; | ||
|
|
||
| let factory = ContractFactory::new(declare_result.class_hash, sender.clone()); | ||
| let mut salt_buffer = [0u8; 32]; | ||
| let mut rng = StdRng::from_entropy(); | ||
| rng.fill_bytes(&mut salt_buffer[1..]); | ||
| let salt = Felt::from_bytes_be(&salt_buffer); | ||
| let unique = true; | ||
| let constructor_calldata = vec![]; | ||
|
|
||
| let deployment_result = factory | ||
| .deploy_v3(constructor_calldata, salt, unique) | ||
| .send() | ||
| .await?; | ||
|
|
||
| wait_for_sent_transaction( | ||
| deployment_result.transaction_hash, | ||
| &test_input.random_paymaster_account.random_accounts()?, | ||
| ) | ||
| .await?; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refactor contract declaration/deployment steps into a helper
The flow of contract declaration, waiting for transaction, and deployment is repeated across test files. Consolidating these routines may reduce duplication and improve clarity.
| let sender = test_input.random_paymaster_account.random_accounts()?; | ||
|
|
||
| let declare_result = sender | ||
| .declare_v3(flattened_sierra_class, compiled_class_hash) | ||
| .send() | ||
| .await?; | ||
|
|
||
| wait_for_sent_transaction( | ||
| declare_result.transaction_hash, | ||
| &test_input.random_paymaster_account.random_accounts()?, | ||
| ) | ||
| .await?; | ||
|
|
||
| let factory = ContractFactory::new(declare_result.class_hash, sender.clone()); | ||
| let mut salt_buffer = [0u8; 32]; | ||
| let mut rng = StdRng::from_entropy(); | ||
| rng.fill_bytes(&mut salt_buffer[1..]); | ||
| let salt = Felt::from_bytes_be(&salt_buffer); | ||
| let unique = true; | ||
| let constructor_calldata = vec![]; | ||
|
|
||
| let deployment_result = factory | ||
| .deploy_v3(constructor_calldata, salt, unique) | ||
| .send() | ||
| .await?; | ||
|
|
||
| wait_for_sent_transaction( | ||
| deployment_result.transaction_hash, | ||
| &test_input.random_paymaster_account.random_accounts()?, | ||
| ) | ||
| .await?; | ||
|
|
||
| let deployment_receipt = test_input | ||
| .random_paymaster_account | ||
| .provider() | ||
| .get_transaction_receipt(deployment_result.transaction_hash) | ||
| .await?; | ||
|
|
||
| let deployed_contract_address = match &deployment_receipt { | ||
| TxnReceipt::Deploy(receipt) => receipt.contract_address, | ||
| TxnReceipt::Invoke(receipt) => { | ||
| if let Some(contract_address) = receipt | ||
| .common_receipt_properties | ||
| .events | ||
| .first() | ||
| .and_then(|event| event.data.first()) | ||
| { | ||
| *contract_address | ||
| } else { | ||
| return Err(OpenRpcTestGenError::CallError( | ||
| CallError::UnexpectedReceiptType, | ||
| )); | ||
| } | ||
| } | ||
| _ => { | ||
| return Err(OpenRpcTestGenError::CallError( | ||
| CallError::UnexpectedReceiptType, | ||
| )); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Encapsulate repetitive deployment processes
Multiple test files use a similar declare→deploy sequence. Extracting these routines into shared helper functions can reduce future maintenance overhead.
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `expected_child` - The expected child child value of edge node | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// `Ok(true)` if the proof is valid, or `Err(ProofError)` if the proof is invalid. | ||
| /// | ||
| /// The verification process works as follows: | ||
| /// | ||
| /// 1. Find the edge node in the Merkle tree with the given expected child hash. | ||
| /// 2. Compute the hash of the edge node. | ||
| /// 3. Iterate over the parents of the edge node until the root of the Merkle tree | ||
| /// is reached. For each parent, check that the child field matches the current | ||
| /// hash, and if so, compute the hash of the parent. | ||
| /// 4. If the final computed root matches the root of the Merkle tree, return | ||
| /// `true`, otherwise return an error. | ||
| pub fn verify_proof(&self, expected_child: &Felt, hash_fn: HashFn) -> Result<bool, ProofError> { | ||
| let edge_node = self | ||
| .find_matching_edge_node(expected_child) | ||
| .ok_or_else(|| ProofError::NoMatchingEdgeNode(*expected_child))?; | ||
| let mut current_node = edge_node; | ||
| let mut current_hash = self.compute_node_hash(current_node, hash_fn)?; | ||
|
|
||
| while let Some(parent_hash) = current_node.parent_hash { | ||
| let parent = self | ||
| .nodes | ||
| .get(&parent_hash) | ||
| .ok_or_else(|| ProofError::NoParentFound(parent_hash))?; | ||
|
|
||
| // Check if the computed hash is equal to any of the parent's children, or if it matches the parent's child field | ||
| match &parent.node { | ||
| MerkleNode::Binary { left, right } => { | ||
| if *left != current_hash && *right != current_hash { | ||
| return Err(ProofError::ChildMismatch { | ||
| computed: current_hash, | ||
| left: *left, | ||
| right: *right, | ||
| node_hash: parent.node_hash, | ||
| }); | ||
| } | ||
| } | ||
| MerkleNode::Edge { child, .. } => { | ||
| if *child != current_hash { | ||
| return Err(ProofError::EdgeChildMismatch { | ||
| computed: current_hash, | ||
| child: *child, | ||
| node_hash: parent.node_hash, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| current_hash = self.compute_node_hash(parent, hash_fn)?; | ||
| current_node = parent; | ||
| } | ||
|
|
||
| Ok(current_hash == self.root) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider returning Result<(), ProofError> instead of Result<bool, ProofError>
Returning Ok(()) on success could unify error handling and remove the boolean success indicator, clarifying the proof-verification outcome.
- pub fn verify_proof(&self, expected_child: &Felt, hash_fn: HashFn) -> Result<bool, ProofError> {
+ pub fn verify_proof(&self, expected_child: &Felt, hash_fn: HashFn) -> Result<(), ProofError> {
...
- Ok(current_hash == self.root)
+ if current_hash == self.root {
+ Ok(())
+ } else {
+ Err( /* relevant error variant */ )
+ }
}Committable suggestion skipped: line range outside the PR's diff.
Uacias
force-pushed
the
v8-get-storage-proof
branch
from
460995c to
9d796e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (6)
openrpc-testgen/src/utils/v7/endpoints/mod.rs (5)
253-265: Address or remove TODO comments and commented code.The codebase contains TODO comments and commented-out code blocks that should be addressed:
get_transaction_receipt_revertimplementation is marked with "TODO: fix that"- Several other commented-out code blocks
Would you like me to help implement the missing functionality or create issues to track these TODOs?
Also applies to: 689-714
35-43: Consider implementing Clone instead of manual URL cloning.The
Rpcstruct's methods frequently clone the URL. Consider implementingClonefor the struct to make the intent clearer and reduce manual cloning.-pub struct Rpc { +#[derive(Clone)] +pub struct Rpc { pub url: Url, } impl Rpc { #[allow(clippy::result_large_err)] pub fn new(url: Url) -> Result<Self, OpenRpcTestGenError> { Ok(Self { url }) } - pub fn set_url(&mut self, new_url: Url) { - self.url = new_url; - } }
795-1429: Improve test function organization and error handling.The test function
test_rpc_endpoints_v0_0_7has several areas for improvement:
- The function is very long (600+ lines) and could be split into smaller, focused test functions
- Error handling is inconsistent - some errors are logged but the function continues, potentially masking issues
- Test cases lack assertions beyond checking if the call succeeds
Consider refactoring into smaller test functions:
async fn test_declare_transactions(rpc: &Rpc, ...) -> Result<(), OpenRpcTestGenError> { // Test declare v2 and v3 } async fn test_invoke_transactions(rpc: &Rpc, ...) -> Result<(), OpenRpcTestGenError> { // Test invoke v1 and v3 } async fn test_block_operations(rpc: &Rpc) -> Result<(), OpenRpcTestGenError> { // Test block_number, get_block_with_txs etc }
60-70: Reduce code duplication in transaction method signatures.Multiple transaction-related methods share identical parameter lists. This violates the DRY principle and makes maintenance harder.
Consider introducing a struct to encapsulate common parameters:
pub struct TransactionParams<'a> { pub sierra_path: &'a str, pub casm_path: &'a str, pub account_class_hash: Option<Felt>, pub account_address: Option<Felt>, pub private_key: Option<Felt>, pub erc20_strk_contract_address: Option<Felt>, pub erc20_eth_contract_address: Option<Felt>, pub amount_per_test: Option<Felt>, }Then update method signatures:
fn add_declare_transaction_v2( &self, params: TransactionParams, ) -> impl std::future::Future<Output = Result<Felt, OpenRpcTestGenError>> + Send;Also applies to: 73-83, 86-96, 99-109, 112-122, 125-135
809-809: Use consistent logging format.The start and end log messages use different emoji styles:
- Start: "⌛ Testing Rpc V7 endpoints -- START ⌛"
- End: "🏁 Testing Devnet V7 endpoints -- END 🏁"
Also, "Rpc" vs "Devnet" terminology is inconsistent.
- info!("{}", "⌛ Testing Rpc V7 endpoints -- START ⌛".yellow()); + info!("{}", "🏁 Testing RPC V7 endpoints -- START 🏁".yellow()); // ... test code ... - info!("{}", "🏁 Testing Devnet V7 endpoints -- END 🏁".yellow()); + info!("{}", "🏁 Testing RPC V7 endpoints -- END 🏁".yellow());Also applies to: 1426-1426
openrpc-testgen/src/utils/v7/endpoints/utils.rs (1)
117-183: Consider refactoring for better maintainability.The function could benefit from the following improvements:
- Split into smaller functions for each validation step
- Extract error messages as constants
- Add more robust validation for addresses
Here's a suggested refactor:
+const ERR_MSG_ACCOUNT_REQUIRED: &str = "Account address is required"; +const ERR_MSG_PRIVATE_KEY_REQUIRED: &str = "Private key is required"; +const ERR_MSG_STRK_REQUIRED: &str = "ERC20 STRK contract address is required"; +const ERR_MSG_ETH_REQUIRED: &str = "ERC20 ETH contract address is required"; +const ERR_MSG_AMOUNT_REQUIRED: &str = "Amount per test is required"; +const ERR_MSG_AMOUNT_POSITIVE: &str = "Amount per test must be greater than zero"; +fn validate_amount(amount: Felt) -> Result<Felt, OpenRpcTestGenError> { + if amount <= Felt::ZERO { + warn!("{}", ERR_MSG_AMOUNT_POSITIVE); + return Err(OpenRpcTestGenError::InvalidInput( + ERR_MSG_AMOUNT_POSITIVE.to_string(), + )); + } + Ok(amount) +} +fn validate_address(address: Felt) -> Result<Felt, OpenRpcTestGenError> { + // Add address validation logic here + Ok(address) +} #[allow(clippy::result_large_err)] pub fn validate_inputs( account_address: Option<Felt>, private_key: Option<Felt>, erc20_strk_contract_address: Option<Felt>, erc20_eth_contract_address: Option<Felt>, amount_per_test: Option<Felt>, ) -> Result<(Felt, Felt, Felt, Felt, Felt), OpenRpcTestGenError> { - match ( - account_address, - private_key, - erc20_strk_contract_address, - erc20_eth_contract_address, - amount_per_test, - ) { - ( - Some(account_address), - Some(private_key), - Some(erc20_strk_contract_address), - Some(erc20_eth_contract_address), - Some(amount_per_test), - ) => { - if amount_per_test <= Felt::ZERO { - warn!("Amount per test must be greater than zero"); - return Err(OpenRpcTestGenError::InvalidInput( - "Amount per test must be greater than zero".to_string(), - )); - }; - Ok(( - account_address, - private_key, - erc20_strk_contract_address, - erc20_eth_contract_address, - amount_per_test, - )) - } - (None, _, _, _, _) => { - warn!("Account address is required"); - Err(OpenRpcTestGenError::InvalidInput( - "Account address is required".to_string(), - )) - } - (_, None, _, _, _) => { - warn!("Private key is required to fund generated account"); - Err(OpenRpcTestGenError::InvalidInput( - "Private key is required".to_string(), - )) - } - (_, _, None, _, _) => { - warn!("ERC20 STRK contract address is required"); - Err(OpenRpcTestGenError::InvalidInput( - "ERC20 STRK contract address is required".to_string(), - )) - } - (_, _, _, None, _) => { - warn!("ERC20 ETH contract address is required"); - Err(OpenRpcTestGenError::InvalidInput( - "ERC20 ETH contract address is required".to_string(), - )) - } - (_, _, _, _, None) => { - warn!("Amount per test is required"); - Err(OpenRpcTestGenError::InvalidInput( - "Amount per test is required".to_string(), - )) - } - } + let account_address = account_address.ok_or_else(|| { + warn!("{}", ERR_MSG_ACCOUNT_REQUIRED); + OpenRpcTestGenError::InvalidInput(ERR_MSG_ACCOUNT_REQUIRED.to_string()) + })?; + + let private_key = private_key.ok_or_else(|| { + warn!("{}", ERR_MSG_PRIVATE_KEY_REQUIRED); + OpenRpcTestGenError::InvalidInput(ERR_MSG_PRIVATE_KEY_REQUIRED.to_string()) + })?; + + let erc20_strk_contract_address = erc20_strk_contract_address.ok_or_else(|| { + warn!("{}", ERR_MSG_STRK_REQUIRED); + OpenRpcTestGenError::InvalidInput(ERR_MSG_STRK_REQUIRED.to_string()) + })?; + + let erc20_eth_contract_address = erc20_eth_contract_address.ok_or_else(|| { + warn!("{}", ERR_MSG_ETH_REQUIRED); + OpenRpcTestGenError::InvalidInput(ERR_MSG_ETH_REQUIRED.to_string()) + })?; + + let amount_per_test = amount_per_test.ok_or_else(|| { + warn!("{}", ERR_MSG_AMOUNT_REQUIRED); + OpenRpcTestGenError::InvalidInput(ERR_MSG_AMOUNT_REQUIRED.to_string()) + })?; + + let amount_per_test = validate_amount(amount_per_test)?; + let account_address = validate_address(account_address)?; + let erc20_strk_contract_address = validate_address(erc20_strk_contract_address)?; + let erc20_eth_contract_address = validate_address(erc20_eth_contract_address)?; + + Ok(( + account_address, + private_key, + erc20_strk_contract_address, + erc20_eth_contract_address, + amount_per_test, + )) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
openrpc-testgen/src/lib.rs(1 hunks)openrpc-testgen/src/utils/v7/endpoints/mod.rs(1 hunks)openrpc-testgen/src/utils/v7/endpoints/utils.rs(1 hunks)openrpc-testgen/src/utils/v8/types.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run cargo_clippy - rust stable
- GitHub Check: Build project - rust stable
🔇 Additional comments (8)
openrpc-testgen/src/utils/v8/types.rs (6)
11-17: Use a more descriptive generic parameter name
RenamingFto something more descriptive likeFieldorBlockKeycan improve clarity for future maintainers and new contributors.
28-29: Unify naming for contract proofs
The fieldscontracts_proofandcontracts_storage_proofsare slightly inconsistent. Consider aligning them (e.g.,contracts_storage_proofvs.contracts_storage_proofs).
45-50: Clarify the rationale for storing bothnodeandnode_hash
Adding a brief doc comment explaining their distinct roles—nodefor child data andnode_hashfor quick lookups—would help others understand this design choice.
199-207: Consider indexing for faster edge node lookup
The linear search is fine for small proofs, but for larger proofs, a dedicated mapping from child-hash to parent-hash could improve performance.
235-235: Fix doc comment typo
Change “storatge” to “storage” in the function documentation.
244-295: Return a unit result instead of a boolean
ReturningOk(())on success would simplify usage, removing the need to check the boolean and unifying error handling.openrpc-testgen/src/lib.rs (1)
35-35: LGTM! Appropriate use of the Clippy lint attribute.The attribute suppresses warnings about the large error type, which is justified as
OpenRpcTestGenErrorneeds to be comprehensive to handle various error cases in the public API.openrpc-testgen/src/utils/v7/endpoints/utils.rs (1)
116-116: LGTM! Appropriate use of the Clippy lint attribute.The attribute suppresses warnings about the large error type, which is justified as
OpenRpcTestGenErrorneeds to be comprehensive to handle various error cases.
| pub block_hash: Felt, | ||
| } | ||
|
|
||
| type HashFn = fn(&Felt, &Felt) -> Felt; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use a trait-based hashing approach
Rather than a raw type HashFn = fn(&Felt, &Felt) -> Felt;, consider a trait to allow more flexible composition of hashing logic. This can simplify testing, especially with different hash strategies.
Summary by CodeRabbit
New Features
Tests