Executable account fix #271
Conversation
WalkthroughThe pull request introduces a comprehensive set of changes across multiple files in a Rust project, focusing on enhancing outside execution functionality, updating dependency management, and refining test infrastructure. The modifications span dependency configurations, contract implementations, utility modules, and test-related components, with a significant emphasis on improving the handling of StarkNet transactions and external execution contexts. Changes
Sequence DiagramsequenceDiagram
participant Client
participant OutsideExecutionModule
participant Account
participant Provider
Client->>OutsideExecutionModule: Prepare Outside Execution
OutsideExecutionModule->>Provider: Get Current Timestamp
Provider-->>OutsideExecutionModule: Return Timestamp
OutsideExecutionModule->>OutsideExecutionModule: Generate Execution Hash
OutsideExecutionModule->>Account: Sign Execution Request
Account-->>OutsideExecutionModule: Return Signed Request
OutsideExecutionModule-->>Client: Prepared Execution Data
Poem
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: 10
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
Cargo.toml(1 hunks)contracts/src/exec_acc.cairo(1 hunks)contracts/src/lib.cairo(1 hunks)openrpc-testgen/src/suite_openrpc/mod.rs(3 hunks)openrpc-testgen/src/suite_openrpc/suite_deploy/suite_contract_calls/test_estimate_message_fee.rs(0 hunks)openrpc-testgen/src/suite_openrpc/test_deploy_account_outside_execution.rs(3 hunks)openrpc-testgen/src/suite_openrpc/test_erc20_transfer_outside_execution.rs(3 hunks)openrpc-testgen/src/utils/mod.rs(1 hunks)openrpc-testgen/src/utils/outside_execution.rs(1 hunks)openrpc-testgen/src/utils/v7/accounts/account/execution.rs(1 hunks)openrpc-testgen/src/utils/v7/endpoints/declare_contract.rs(2 hunks)openrpc-testgen/src/utils/v7/endpoints/errors.rs(1 hunks)openrpc-testgen/src/utils/v7/endpoints/mod.rs(4 hunks)t9n/Cargo.toml(1 hunks)
💤 Files with no reviewable changes (1)
- openrpc-testgen/src/suite_openrpc/suite_deploy/suite_contract_calls/test_estimate_message_fee.rs
⏰ 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 (27)
contracts/src/exec_acc.cairo (6)
1-3: No issues with license specification and version annotation.
Everything looks correct here.
4-13: Module definition seems appropriate.
DeclaringMyAccountExecas a StarkNet account contract is straightforward and aligns with OpenZeppelin's recommended structure.
14-27: External implementations appear consistent with the OpenZeppelin account extensions.
All references toAccountComponentand other SRC components are logically grouped and follow standard naming conventions. Ensure that each external interface (e.g.,SRC6Impl,SRC6CamelOnlyImpl, etc.) is needed for your use case to avoid unnecessary code bloat.
28-30: Internal implementations are cleanly separated.
Defining internal implementations for account and outside execution separately helps maintain clarity. Confirm that you’re actually utilizing these internals in your contract code.
32-41: Substorage structure is well-defined but verify versioning strategy.
The substorage references each component atv0, which is fine, though you might want a strategy for future migrations if you plan to update these components.
42-52: Flattened event enumeration is well-structured.
Combining events from different components is a convenient approach. No observed issues here.openrpc-testgen/src/utils/outside_execution.rs (3)
1-10: Imports are properly declared.
All required modules (hashing, crypto, etc.) are correctly brought into scope. No issues found here.
12-18: Constant definitions for type hashes.
Defining typed hashes for domain, call, and outside execution fosters clarity. Ensure these values match your contract definitions for correct hashing.
109-124: Timestamp retrieval is straightforward.
Grabbing the timestamp from the latest or pending block works as intended. This is essential for executing time-bound outside transactions. No major concerns here.openrpc-testgen/src/suite_openrpc/test_erc20_transfer_outside_execution.rs (6)
6-6: Import usage is correct.
Bringing inget_current_timestamp,prepare_outside_execution, andOutsideExecutionfrom the utils module aligns with the new outside exec flow.
187-188: Retrieving current timestamp is suitable for scheduling.
Acquiring the timestamp ensures theexecute_afterandexecute_beforefields inOutsideExecutionare accurately set.
190-197: Nonce retrieval from the provider is appropriate.
Using the provider to fetch the nonce helps avoid stale or collision-prone values. Be mindful of transaction ordering with a multi-account scenario.
199-205: OutsideExecution struct fields are properly set.
The dynamic fieldsexecute_beforeandexecute_afterare well-defined for time constraint logic, plus a fresh nonce. No immediate issues.
207-219: prepare_outside_execution simplifies calldata generation.
Calling a shared utility to compute the hash and signature is safer than manually building the ECDSA. This reduces repeated code and potential security flaws.
227-227: Updated call selector to 'execute_from_outside_v2'.
Renaming the contract method implies a revised contract flow. Verify that the new method is deployed/available to avoid "missing selector" errors at runtime.openrpc-testgen/src/utils/v7/endpoints/errors.rs (1)
67-68: Good addition to improve error granularity.Introducing
EcdsaSignErrorcleanly encapsulates issues arising from ECDSA signatures, aiding more precise error handling.openrpc-testgen/src/suite_openrpc/test_deploy_account_outside_execution.rs (7)
3-15: Imports appear consistent and necessary.The inclusion of new utilities from
outside_executionaligns with the revised logic for timestamp handling. No issues found.
20-22: No actionable remarks.These lines introduce or separate imports; no direct functionality changes.
40-48: Nonce fetching logic looks appropriate.The code properly awaits the nonce and propagates errors using the
?operator, ensuring safe error handling.
63-65: Timestamp retrieval is well-handled.Storing and reusing the timestamp from the provider is sound; consider edge cases if the environment clock is skewed, though likely acceptable in test settings.
68-70: Revisit time constraints for outside execution.
execute_beforeset to a future time andexecute_afterset in the past may be intentional for tests, but confirm these constraints don’t inadvertently break real use cases.
74-87: Outside execution preparation is logically consistent.Parameters and chaining of
await?suggest robust error propagation. The approach cleanly centralizes how the call’s calldata is formed.
93-93: Confirm new function selector usage.Switching to
execute_from_outside_v2matches the updated logic. Ensure any references to the old function are removed if obsolete.openrpc-testgen/src/suite_openrpc/mod.rs (2)
93-93: LGTM: Added field for storing executable account's private key.Good addition that enables reuse of the private key across test cases.
259-259: LGTM: Properly storing the executable account's private key.The private key is correctly extracted from the signing key using
secret_scalar().openrpc-testgen/src/utils/v7/endpoints/declare_contract.rs (1)
153-187: LGTM: Added utility function to get raw contract strings.Good addition that provides raw access to contract files without parsing them. The function:
- Reuses existing error handling patterns
- Follows DRY principle
- Provides flexibility for cases where raw strings are needed
openrpc-testgen/src/utils/mod.rs (1)
4-4: LGTM: Added new module for outside execution.Good addition that follows Rust module organization conventions.
| #[constructor] | ||
| fn constructor(ref self: ContractState, public_key: felt252) { | ||
| self.account.initializer(public_key); | ||
| self.src9.initializer(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Constructor omits initialization of SRC5.
You're initializing account and src9 substorage, but not src5. If src5 requires initialization, consider adding it here. Otherwise, ensure it’s intentionally left out.
You could add a line if SRC5 also needs an initializer:
#[constructor]
fn constructor(ref self: ContractState, public_key: felt252) {
self.account.initializer(public_key);
self.src9.initializer();
+ // self.src5.initializer(); // Uncomment if needed
}📝 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.
| #[constructor] | |
| fn constructor(ref self: ContractState, public_key: felt252) { | |
| self.account.initializer(public_key); | |
| self.src9.initializer(); | |
| } | |
| } | |
| #[constructor] | |
| fn constructor(ref self: ContractState, public_key: felt252) { | |
| self.account.initializer(public_key); | |
| self.src9.initializer(); | |
| // self.src5.initializer(); // Uncomment if needed | |
| } | |
| } |
| #[derive(Debug, CairoSerde)] | ||
| pub struct OutsideExecution { | ||
| pub caller: Felt, | ||
| pub nonce: Felt, | ||
| pub execute_after: u64, | ||
| pub execute_before: u64, | ||
| pub calls: Vec<Call>, | ||
| } | ||
|
|
||
| #[derive(Debug, CairoSerde)] | ||
| pub struct StarknetDomain { | ||
| pub name: Felt, | ||
| pub version: Felt, | ||
| pub chain_id: Felt, | ||
| pub revision: Felt, | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Struct definitions are clearly laid out.
OutsideExecution and StarknetDomain neatly encapsulate domain and execution details. Consider adding inline documentation to clarify field meanings if used widely.
| pub fn get_starknet_domain_hash(chain_id: Felt) -> Felt { | ||
| let domain = StarknetDomain { | ||
| name: Felt::from_bytes_be_slice(b"Account.execute_from_outside"), | ||
| version: Felt::TWO, | ||
| chain_id, | ||
| revision: Felt::ONE, | ||
| }; | ||
|
|
||
| let domain_vec = vec![ | ||
| STARKNET_DOMAIN_TYPE_HASH, | ||
| domain.name, | ||
| domain.version, | ||
| domain.chain_id, | ||
| domain.revision, | ||
| ]; | ||
| poseidon_hash_many(&domain_vec) | ||
| } | ||
|
|
||
| pub fn get_outside_execution_hash(outside_execution: &OutsideExecution) -> Felt { | ||
| let calls_vec = outside_execution.calls.clone(); | ||
| let mut hashed_calls = Vec::<Felt>::new(); | ||
|
|
||
| for call in calls_vec { | ||
| hashed_calls.push(get_call_hash(call)); | ||
| } | ||
|
|
||
| let mut hasher_outside_execution = PoseidonHasher::new(); | ||
| hasher_outside_execution.update(OUTSIDE_EXECUTION_TYPE_HASH); | ||
| hasher_outside_execution.update(outside_execution.caller); | ||
| hasher_outside_execution.update(outside_execution.nonce); | ||
| hasher_outside_execution.update(Felt::from(outside_execution.execute_after)); | ||
| hasher_outside_execution.update(Felt::from(outside_execution.execute_before)); | ||
| hasher_outside_execution.update(poseidon_hash_many(&hashed_calls)); | ||
|
|
||
| hasher_outside_execution.finalize() | ||
| } | ||
|
|
||
| pub fn get_call_hash(call: Call) -> Felt { | ||
| let mut hasher_call = PoseidonHasher::new(); | ||
| hasher_call.update(CALL_TYPE_HASH); | ||
| hasher_call.update(call.to); | ||
| hasher_call.update(call.selector); | ||
| hasher_call.update(poseidon_hash_many(&call.calldata)); | ||
| hasher_call.finalize() | ||
| } | ||
|
|
||
| pub async fn prepare_outside_execution( | ||
| outside_execution: &OutsideExecution, | ||
| signer_address: Felt, | ||
| signer_private_key: Felt, | ||
| chain_id: Felt, | ||
| ) -> Result<Vec<Felt>, OpenRpcTestGenError> { | ||
| let mut final_hasher = PoseidonHasher::new(); | ||
| final_hasher.update(Felt::from_bytes_be_slice(b"StarkNet Message")); | ||
| final_hasher.update(get_starknet_domain_hash(chain_id)); | ||
| final_hasher.update(signer_address); | ||
| final_hasher.update(get_outside_execution_hash(outside_execution)); | ||
|
|
||
| let hash = final_hasher.finalize(); | ||
|
|
||
| let starknet::core::crypto::ExtendedSignature { r, s, v: _ } = | ||
| ecdsa_sign(&signer_private_key, &hash)?; | ||
|
|
||
| let outside_execution_cairo_serialized = OutsideExecution::cairo_serialize(outside_execution); | ||
|
|
||
| let mut calldata_to_executable_account_call = outside_execution_cairo_serialized.clone(); | ||
| calldata_to_executable_account_call.push(Felt::from_dec_str("2")?); | ||
| calldata_to_executable_account_call.push(r); | ||
| calldata_to_executable_account_call.push(s); | ||
|
|
||
| Ok(calldata_to_executable_account_call) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Hash-related utility functions are well-structured, but avoid cloning the calls vector.
The hashing logic appears correct, mixing typed hashes with Poseidon. However, in get_outside_execution_hash, cloning outside_execution.calls is unnecessary and can be refactored for performance:
-pub fn get_outside_execution_hash(outside_execution: &OutsideExecution) -> Felt {
- let calls_vec = outside_execution.calls.clone();
- let mut hashed_calls = Vec::<Felt>::new();
- for call in calls_vec {
+pub fn get_outside_execution_hash(outside_execution: &OutsideExecution) -> Felt {
+ let mut hashed_calls = Vec::<Felt>::with_capacity(outside_execution.calls.len());
+ for call in &outside_execution.calls {
hashed_calls.push(get_call_hash(call.clone()));
}
...
}📝 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.
| pub fn get_starknet_domain_hash(chain_id: Felt) -> Felt { | |
| let domain = StarknetDomain { | |
| name: Felt::from_bytes_be_slice(b"Account.execute_from_outside"), | |
| version: Felt::TWO, | |
| chain_id, | |
| revision: Felt::ONE, | |
| }; | |
| let domain_vec = vec![ | |
| STARKNET_DOMAIN_TYPE_HASH, | |
| domain.name, | |
| domain.version, | |
| domain.chain_id, | |
| domain.revision, | |
| ]; | |
| poseidon_hash_many(&domain_vec) | |
| } | |
| pub fn get_outside_execution_hash(outside_execution: &OutsideExecution) -> Felt { | |
| let calls_vec = outside_execution.calls.clone(); | |
| let mut hashed_calls = Vec::<Felt>::new(); | |
| for call in calls_vec { | |
| hashed_calls.push(get_call_hash(call)); | |
| } | |
| let mut hasher_outside_execution = PoseidonHasher::new(); | |
| hasher_outside_execution.update(OUTSIDE_EXECUTION_TYPE_HASH); | |
| hasher_outside_execution.update(outside_execution.caller); | |
| hasher_outside_execution.update(outside_execution.nonce); | |
| hasher_outside_execution.update(Felt::from(outside_execution.execute_after)); | |
| hasher_outside_execution.update(Felt::from(outside_execution.execute_before)); | |
| hasher_outside_execution.update(poseidon_hash_many(&hashed_calls)); | |
| hasher_outside_execution.finalize() | |
| } | |
| pub fn get_call_hash(call: Call) -> Felt { | |
| let mut hasher_call = PoseidonHasher::new(); | |
| hasher_call.update(CALL_TYPE_HASH); | |
| hasher_call.update(call.to); | |
| hasher_call.update(call.selector); | |
| hasher_call.update(poseidon_hash_many(&call.calldata)); | |
| hasher_call.finalize() | |
| } | |
| pub async fn prepare_outside_execution( | |
| outside_execution: &OutsideExecution, | |
| signer_address: Felt, | |
| signer_private_key: Felt, | |
| chain_id: Felt, | |
| ) -> Result<Vec<Felt>, OpenRpcTestGenError> { | |
| let mut final_hasher = PoseidonHasher::new(); | |
| final_hasher.update(Felt::from_bytes_be_slice(b"StarkNet Message")); | |
| final_hasher.update(get_starknet_domain_hash(chain_id)); | |
| final_hasher.update(signer_address); | |
| final_hasher.update(get_outside_execution_hash(outside_execution)); | |
| let hash = final_hasher.finalize(); | |
| let starknet::core::crypto::ExtendedSignature { r, s, v: _ } = | |
| ecdsa_sign(&signer_private_key, &hash)?; | |
| let outside_execution_cairo_serialized = OutsideExecution::cairo_serialize(outside_execution); | |
| let mut calldata_to_executable_account_call = outside_execution_cairo_serialized.clone(); | |
| calldata_to_executable_account_call.push(Felt::from_dec_str("2")?); | |
| calldata_to_executable_account_call.push(r); | |
| calldata_to_executable_account_call.push(s); | |
| Ok(calldata_to_executable_account_call) | |
| } | |
| pub fn get_starknet_domain_hash(chain_id: Felt) -> Felt { | |
| let domain = StarknetDomain { | |
| name: Felt::from_bytes_be_slice(b"Account.execute_from_outside"), | |
| version: Felt::TWO, | |
| chain_id, | |
| revision: Felt::ONE, | |
| }; | |
| let domain_vec = vec![ | |
| STARKNET_DOMAIN_TYPE_HASH, | |
| domain.name, | |
| domain.version, | |
| domain.chain_id, | |
| domain.revision, | |
| ]; | |
| poseidon_hash_many(&domain_vec) | |
| } | |
| pub fn get_outside_execution_hash(outside_execution: &OutsideExecution) -> Felt { | |
| let mut hashed_calls = Vec::<Felt>::with_capacity(outside_execution.calls.len()); | |
| for call in &outside_execution.calls { | |
| hashed_calls.push(get_call_hash(call.clone())); | |
| } | |
| let mut hasher_outside_execution = PoseidonHasher::new(); | |
| hasher_outside_execution.update(OUTSIDE_EXECUTION_TYPE_HASH); | |
| hasher_outside_execution.update(outside_execution.caller); | |
| hasher_outside_execution.update(outside_execution.nonce); | |
| hasher_outside_execution.update(Felt::from(outside_execution.execute_after)); | |
| hasher_outside_execution.update(Felt::from(outside_execution.execute_before)); | |
| hasher_outside_execution.update(poseidon_hash_many(&hashed_calls)); | |
| hasher_outside_execution.finalize() | |
| } | |
| pub fn get_call_hash(call: Call) -> Felt { | |
| let mut hasher_call = PoseidonHasher::new(); | |
| hasher_call.update(CALL_TYPE_HASH); | |
| hasher_call.update(call.to); | |
| hasher_call.update(call.selector); | |
| hasher_call.update(poseidon_hash_many(&call.calldata)); | |
| hasher_call.finalize() | |
| } | |
| pub async fn prepare_outside_execution( | |
| outside_execution: &OutsideExecution, | |
| signer_address: Felt, | |
| signer_private_key: Felt, | |
| chain_id: Felt, | |
| ) -> Result<Vec<Felt>, OpenRpcTestGenError> { | |
| let mut final_hasher = PoseidonHasher::new(); | |
| final_hasher.update(Felt::from_bytes_be_slice(b"StarkNet Message")); | |
| final_hasher.update(get_starknet_domain_hash(chain_id)); | |
| final_hasher.update(signer_address); | |
| final_hasher.update(get_outside_execution_hash(outside_execution)); | |
| let hash = final_hasher.finalize(); | |
| let starknet::core::crypto::ExtendedSignature { r, s, v: _ } = | |
| ecdsa_sign(&signer_private_key, &hash)?; | |
| let outside_execution_cairo_serialized = OutsideExecution::cairo_serialize(outside_execution); | |
| let mut calldata_to_executable_account_call = outside_execution_cairo_serialized.clone(); | |
| calldata_to_executable_account_call.push(Felt::from_dec_str("2")?); | |
| calldata_to_executable_account_call.push(r); | |
| calldata_to_executable_account_call.push(s); | |
| Ok(calldata_to_executable_account_call) | |
| } |
| @@ -510,7 +510,7 @@ where | |||
| }, | |||
| }; | |||
| let invoke = prepared | |||
| .get_invoke_request(true, skip_signature) | |||
| .get_invoke_request(false, skip_signature) | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Clarify boolean arguments for better readability.
Calling get_invoke_request(false, skip_signature) might reduce clarity without a descriptive wrapper or named parameter. Consider refactoring to clearly convey the intent of the boolean parameters.
| PathBuf::from_str("target/dev/contracts_MyAccountExec.contract_class.json")?, | ||
| PathBuf::from_str( | ||
| "target/dev/contracts_ExecutableAccount.compiled_contract_class.json", | ||
| "target/dev/contracts_MyAccountExec.compiled_contract_class.json", | ||
| )?, |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent contract naming across test suites
The contract name has been changed to MyAccountExec in suite_openrpc/mod.rs, but the old name ExecutableAccount is still used in several other test suites:
suite_katana/mod.rssuite_katana_no_mining/mod.rssuite_katana_no_fee/mod.rssuite_katana_no_account_validation/mod.rs
🔗 Analysis chain
Contract path updated to use new naming convention.
The contract paths have been updated from ExecutableAccount to MyAccountExec, reflecting the new naming convention.
Let's verify the contract files exist:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new contract files
fd --type f "MyAccountExec" target/dev/
Length of output: 138
Script:
#!/bin/bash
# Search for both old and new contract names, and path configurations
echo "=== Searching for contract names ==="
rg -l "ExecutableAccount|MyAccountExec"
echo -e "\n=== Searching for path configurations ==="
fd -e toml -e json . | xargs rg "target/dev|contracts_.*\.json" || true
echo -e "\n=== Checking for contract-related constants or configurations ==="
rg "PathBuf::from_str.*contract.*json"
Length of output: 6720
| // #[allow(clippy::too_many_arguments)] | ||
| // fn invoke_contract_erc20_transfer( | ||
| // &self, | ||
| // sierra_path: &str, | ||
| // casm_path: &str, | ||
| // account_class_hash: Option<Felt>, | ||
| // 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>, | ||
| // ) -> impl std::future::Future<Output = Result<Felt, OpenRpcTestGenError>>; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider cleaning up commented out code.
The invoke_contract_erc20_transfer function and its implementation have been commented out. If this functionality is no longer needed, consider removing the code entirely rather than leaving it commented out. If it's needed for future reference, add a comment explaining why it's preserved.
Also applies to: 307-330
| // match rpc | ||
| // .invoke_contract_erc20_transfer( | ||
| // sierra_path, | ||
| // casm_path, | ||
| // class_hash, | ||
| // account_address, | ||
| // private_key, | ||
| // erc20_strk_contract_address, | ||
| // erc20_eth_contract_address, | ||
| // amount_per_test, | ||
| // ) | ||
| // .await | ||
| // { | ||
| // Ok(_) => { | ||
| // info!( | ||
| // "{} {}", | ||
| // "\n✓ Rpc Test paymaster via invoke erc20 transfer COMPATIBLE".green(), | ||
| // "✓".green() | ||
| // ) | ||
| // } | ||
| // Err(e) => error!( | ||
| // "{} {} {}", | ||
| // "✗ Rpc Test paymaster via invoke erc20 transfer INCOMPATIBLE:".red(), | ||
| // e.to_string().red(), | ||
| // "✗".red() | ||
| // ), | ||
| // } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing commented out test code.
The test for invoke_contract_erc20_transfer has been commented out. Since the related functionality is also commented out, consider removing this test code as well to maintain consistency.
| @@ -5,7 +5,7 @@ version.workspace = true | |||
|
|
|||
| [dependencies] | |||
| clap.workspace = true | |||
| crypto-utils.workspace = true | |||
| crypto-utils = { path = "../crypto-utils" } | |||
There was a problem hiding this comment.
Align crypto-utils dependency with workspace configuration.
The main workspace's Cargo.toml uses a git dependency for crypto-utils, but this file uses a local path. This inconsistency could lead to version mismatches.
Apply this diff to align with the workspace configuration:
-crypto-utils = { path = "../crypto-utils" }
+crypto-utils = { git = "https://github.com/neotheprogramist/starknet-rpc-tests.git", rev = "824a4c294d5040f73fd576d0ed17ba85439fc593" }📝 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.
| crypto-utils = { path = "../crypto-utils" } | |
| crypto-utils = { git = "https://github.com/neotheprogramist/starknet-rpc-tests.git", rev = "824a4c294d5040f73fd576d0ed17ba85439fc593" } |
| @@ -26,6 +26,7 @@ mod smpl16; | |||
| mod smpl17; | |||
| mod smpl18; | |||
| mod smpl19; | |||
| mod exec_acc; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider moving exec_acc module into the paymaster block.
The new exec_acc module appears to be account-related, similar to account_executable and account_oz which are currently organized within the paymaster module block. Consider moving it there for better code organization and consistency.
| @@ -30,7 +30,7 @@ clap = { version = "4.5.16", features = ["derive", "env"] } | |||
| clap_derive = "4.5.13" | |||
| colored = "2.1.0" | |||
| crypto-bigint = "0.5.5" | |||
| crypto-utils = { path = "./crypto-utils" } | |||
| crypto-utils = { git = "https://github.com/neotheprogramist/starknet-rpc-tests.git", rev = "824a4c294d5040f73fd576d0ed17ba85439fc593" } | |||
There was a problem hiding this comment.
💡 Codebase verification
Security concern: Using test framework's crypto-utils in production
The crypto-utils dependency is part of a test framework repository and handles critical cryptographic operations. This raises several concerns:
- The repository lacks proper versioning and release tags
- The package is primarily designed for testing purposes
- No clear indication of production-readiness or security audits
Recommendations:
- Use a well-established, production-ready cryptographic library
- If custom implementation is required, move the crypto-utils to a dedicated production repository with proper versioning
- Ensure thorough security audits before using in production
🔗 Analysis chain
Verify the stability of the crypto-utils dependency.
The crypto-utils dependency now points to a repository named "starknet-rpc-tests", which suggests it might be a testing repository. Please verify:
- Is this the correct production-ready repository for crypto-utils?
- Is the specified commit stable and suitable for production use?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the crypto-utils repository and commit
# Check if the repository exists and is accessible
gh repo view neotheprogramist/starknet-rpc-tests
# Get information about the specific commit
gh api repos/neotheprogramist/starknet-rpc-tests/commits/824a4c294d5040f73fd576d0ed17ba85439fc593 --jq '.commit.message'
# Check if this is the latest commit
gh api repos/neotheprogramist/starknet-rpc-tests/commits --jq '.[0].sha'
Length of output: 4627
Script:
#!/bin/bash
# Check for tags and releases
gh api repos/neotheprogramist/starknet-rpc-tests/tags --jq '.[].name'
# Search for crypto-utils related files and documentation
fd -t f -e rs -e md crypto
rg -i "crypto.utils" -A 5 -B 5
Length of output: 16064
Summary by CodeRabbit
Release Notes
Dependency Updates
crypto-utilsdependency to use a remote Git repository in multiple projectsNew Features
MyAccountExeccontract module with enhanced account management capabilitiesImprovements
Changes
invoke_contract_erc20_transferfunction from RPC endpoints