Add walnut flag to sozo execute and migrate apply commands #2333
Conversation
|
Could you please advise on the most appropriate way to pass the RPC URL to the following location in the code: /~https://github.com/walnuthq/dojo/blob/f4e27124e0127a0f6d3fe025c08784899e294692/crates/sozo/ops/src/utils.rs#L114 |
|
We are working with @barabanovro and walnut team to integrate walnut debugger on dojo yes. The idea is to provide a way users can debug step by step their contract execution. @barabanovro however this PR depends on the branch on my fork that is containing the debug info from the sierra. I'll open a branch tomorrow on dojo and your PR should target this branch. 👍 |
|
I have made changes to pass the RPC URL from |
|
@barabanovro just changed the base branch, which is rebased on main. 👍 |
barabanovro
changed the title
| #[arg(long)] | ||
| #[arg(help = "If --wait is set, the link to debug the transaction with Walnut is displayed \ | ||
| on stdout. Otherwise, it is a no-op.")] | ||
| #[arg(global = true)] | ||
| pub walnut: bool, |
There was a problem hiding this comment.
i think we should make setting --walnut also implies using --wait without having to specify it manually.
|
I can't lint my code with error: first doc comment paragraph is too long
--> crates/katana/tasks/src/lib.rs:92:1
|
92 | / /// A thread-pool for spawning blocking tasks . This is a simple wrapper around *rayon*'s
93 | | /// thread-pool. This is mainly for executing expensive CPU-bound tasks. For spawing blocking
94 | | /// IO-bound tasks, use [TokioTaskSpawner::spawn_blocking] instead.
95 | | ///
96 | | /// Refer to the [CPU-bound tasks and blocking code] section of the *tokio* docs and this [blog
| |_
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#too_long_first_doc_paragraph
= note: `-D clippy::too-long-first-doc-paragraph` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::too_long_first_doc_paragraph)]`
error: could not compile `katana-tasks` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish... |
|
@kariy, thanks for the comments, I have addressed them. The PR is ready for review. I am still debugging the issue with inconsistent class hashes, however, no significant changes are expected. |
Should be now fixed if you rebase. |
|
@barabanovro can you pls rebase to main after that we should be good to go. the debug stuff have been merged on main #2392. |
WalkthroughOhayo, sensei! The changes introduce the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Sozo
participant WalnutAPI
participant RPC
User->>Sozo: Execute command with --walnut flag
Sozo->>WalnutAPI: Upload source code for verification
WalnutAPI->>Sozo: Verification result
Sozo->>RPC: Handle transaction
RPC->>Sozo: Transaction result
Sozo->>User: Display result and debug link
Possibly related PRs
Recent review detailsConfiguration used: .coderabbit.yaml Files ignored due to path filters (3)
Files selected for processing (7)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (4)
Additional comments not posted (4)
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
Documentation and Community
|
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range, codebase verification and nitpick comments (5)
crates/sozo/ops/src/account.rs (1)
Line range hint
251-281: Ensure consistent handling of thewalnutflag in transaction configurations.Ohayo, sensei! The addition of the
walnutflag to theTxnAction::Sendvariant is a crucial update. It's important to ensure that this flag is consistently handled across different parts of the transaction configuration to avoid any discrepancies or unintended behaviors.Consider verifying that the
walnutflag is appropriately used in all relevant parts of the transaction process, particularly in how it affects the transaction's execution and finality conditions. Here's a snippet to illustrate proper integration:TxnAction::Send { wait, receipt, max_fee_raw, fee_estimate_multiplier, walnut } => { let txn_config = TxnConfig { fee_estimate_multiplier, wait, receipt, max_fee_raw, walnut }; + // Ensure walnut flag is considered in transaction execution logic + if walnut { + // Specific logic for walnut-enabled transactions + } do_account_deploy( max_fee, txn_config, target_deployment_address, no_confirmation, account_deployment, &provider, poll_interval, &mut account, ) .await?; }This modification ensures that the
walnutflag's presence is actively considered during the deployment process.crates/dojo-lang/src/compiler.rs (3)
Line range hint
91-184: Refactor suggestion for the main compilation loop.The main compilation loop in the
compilemethod has a TODO comment about simplifying and optimizing the loop. This is crucial as it directly impacts the performance and maintainability of the compilation process.Consider breaking down the loop into smaller, more manageable functions or utilizing more efficient data structures or algorithms to enhance clarity and performance.
Line range hint
147-184: Enhanced error handling and diagnostics in contract validation.The error handling in the contract validation step is robust, providing detailed diagnostics about unsupported library functions. However, it could be further improved by externalizing error messages or using a dedicated error handling framework to manage different types of contract validation errors more systematically.
Refactor the error handling to use a dedicated module or service that handles different types of errors and can be easily extended or modified without changing the core compilation logic.
- ws.config().ui().warn(diagnostic); + contract_validator.warn(diagnostic); // Assuming `contract_validator` is an abstraction over different validation scenarios
Line range hint
276-433: Review of theupdate_filesfunction and related artifact handling.The
update_filesfunction has been significantly modified to handle the new debug information structure. The changes are well-integrated, but there's a potential improvement in how artifacts are managed and saved.Consider abstracting the artifact handling into a separate class or module to reduce the complexity of the
update_filesfunction and improve the separation of concerns.- compiled_artifacts: HashMap<String, (Felt, ContractClass, Option<SierraToCairoDebugInfo>)>, + compiled_artifacts: CompiledArtifacts // Where CompiledArtifacts is a new class handling the specifics of artifact managementcrates/sozo/ops/src/utils.rs (1)
Line range hint
80-123: Ohayo, sensei! Enhanced debugging capabilities in transaction handling.The addition of
rpc_urlanddebug_with_walnutparameters to thehandle_transaction_resultfunction is a commendable enhancement. It allows developers to optionally debug transactions using Walnut, which can significantly aid in troubleshooting and ensuring transaction integrity.The conditional logic to check if
debug_with_walnutis true and then attempt to use therpc_urlto debug the transaction is well-implemented. This ensures that the debugging feature is optional and does not affect the existing functionality if not used.However, it would be beneficial to ensure that the
rpc_urlis not just present but also valid before attempting to use it. Adding a simple validation check could prevent potential runtime errors or confusion during debugging sessions.Consider adding a validation step for
rpc_urlto ensure it is not only present but also a valid URL before attempting to debug with Walnut. This could be done using theurl::Url::parsemethod to catch any malformed URLs early in the process.
| account_address = "0x6162896d1d7ab204c7ccac6dd5f8e9e7c25ecd5ae4fcb4ad32e57786bb46e03" | ||
| private_key = "0x1800000000300000180000000000030000000000003006001800006600" | ||
| account_address = "0x7af481c87d29af6940a23c28467b8d0194f88bf7f0e5cb344eaeb9076b90b03" | ||
| private_key = "0x4fe70c28d3eeb57610b31b95a04e7b468dca5f986ac57d62c7b216db820e8af" |
There was a problem hiding this comment.
Ohayo, sensei! Security concern with the exposed private key.
The update to the private_key is noted, but exposing private keys in configuration files poses a significant security risk. Consider using environment variables or a secure vault solution.
Tools
Gitleaks
16-16: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
| /// Prints a URL to the Walnut page for debugging the transaction. | ||
| /// Only supported on hosted networks (non-localhost). | ||
| pub fn walnut_debug_transaction(ui: &Ui, rpc_url: &Url, transaction_hash: &Felt) { | ||
| // Check if the RPC URL is not localhost | ||
| if rpc_url.host_str() != Some("localhost") && rpc_url.host_str() != Some("127.0.0.1") { | ||
| // Encode the RPC URL | ||
| let encoded_rpc_url = encode(rpc_url.as_str()); | ||
| ui.print(format!( | ||
| "Debug transaction with Walnut: {}/transactions?rpcUrl={}&txHash={:#066x}", | ||
| WALNUT_APP_URL, encoded_rpc_url, transaction_hash | ||
| )); | ||
| } else { | ||
| ui.warn("Debugging transactions with Walnut is only supported on hosted networks"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Walnut debugging function looks good.
The implementation of walnut_debug_transaction is approved. Consider adding more detailed documentation about when and how to use this function effectively.
Would you like help with drafting the documentation?
| @@ -18,6 +18,7 @@ pub struct TxnConfig { | |||
| pub wait: bool, | |||
| pub receipt: bool, | |||
| pub max_fee_raw: Option<Felt>, | |||
| pub walnut: bool, | |||
There was a problem hiding this comment.
Ohayo, sensei! Consider setting a default value for walnut.
The addition of the walnut field in TxnConfig is a wise move to enhance functionality. However, it's crucial to ensure that this field is initialized properly to avoid any undefined behavior. Consider setting a default value in the Default trait implementation for TxnConfig.
| /// Verifies all classes declared during migration. | ||
| /// Only supported on hosted networks (non-localhost). | ||
| /// | ||
| /// This function verifies all contracts and models in the strategy. For every contract and model, | ||
| /// it sends a request to the Walnut backend with the class name, class hash, RPC URL, and source | ||
| /// code. Walnut will then build the project with Sozo, compare the Sierra bytecode with the | ||
| /// bytecode on the network, and if they are equal, it will store the source code and associate it | ||
| /// with the class hash. | ||
| pub async fn walnut_verify_migration_strategy( | ||
| ws: &Workspace<'_>, | ||
| rpc_url: String, | ||
| migration_strategy: &MigrationStrategy, | ||
| ) -> Result<()> { | ||
| let ui = ws.config().ui(); | ||
| // Check if rpc_url is localhost | ||
| if rpc_url.contains("localhost") || rpc_url.contains("127.0.0.1") { | ||
| ui.print(" "); | ||
| ui.warn("Verifying classes with Walnut is only supported on hosted networks."); | ||
| ui.print(" "); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // its path to a file so `parent` should never return `None` | ||
| let root_dir: &Path = ws.manifest_path().parent().unwrap().as_std_path(); | ||
| let default_namespace = get_default_namespace_from_ws(ws)?; | ||
|
|
||
| // Check if there are any contracts or models in the strategy | ||
| if migration_strategy.contracts.is_empty() && migration_strategy.models.is_empty() { | ||
| ui.print(" "); | ||
| ui.print("🌰 No contracts or models to verify."); | ||
| ui.print(" "); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Notify start of verification | ||
| ui.print(" "); | ||
| ui.print("🌰 Verifying classes with Walnut..."); | ||
| ui.print(" "); | ||
|
|
||
| // Retrieve the API key and URL from environment variables | ||
| let api_key = walnut_get_api_key()?; | ||
| let api_url = walnut_get_api_url(); | ||
|
|
||
| // Collect source code | ||
| let source_code = collect_source_code(root_dir)?; | ||
|
|
||
| // Prepare verification payloads | ||
| let mut verification_tasks = Vec::new(); | ||
| let mut class_tags = Vec::new(); | ||
|
|
||
| for contract_migration in &migration_strategy.contracts { | ||
| let class_name = get_class_name_from_artifact_path( | ||
| &contract_migration.artifact_path, | ||
| &default_namespace, | ||
| )?; | ||
| let verification_payload = VerificationPayload { | ||
| class_name: class_name.clone(), | ||
| class_hash: contract_migration.diff.local_class_hash.to_hex_string(), | ||
| rpc_url: rpc_url.clone(), | ||
| source_code: source_code.clone(), | ||
| }; | ||
| class_tags.push(contract_migration.diff.tag.clone()); | ||
| verification_tasks.push(verify_class(verification_payload, &api_url, &api_key)); | ||
| } | ||
|
|
||
| for class_migration in &migration_strategy.models { | ||
| let class_name = | ||
| get_class_name_from_artifact_path(&class_migration.artifact_path, &default_namespace)?; | ||
| let verification_payload = VerificationPayload { | ||
| class_name: class_name.clone(), | ||
| class_hash: class_migration.diff.local_class_hash.to_hex_string(), | ||
| rpc_url: rpc_url.clone(), | ||
| source_code: source_code.clone(), | ||
| }; | ||
| class_tags.push(class_migration.diff.tag.clone()); | ||
| verification_tasks.push(verify_class(verification_payload, &api_url, &api_key)); | ||
| } | ||
|
|
||
| // Run all verification tasks | ||
| let results = join_all(verification_tasks).await; | ||
|
|
||
| for (i, result) in results.into_iter().enumerate() { | ||
| match result { | ||
| Ok(message) => { | ||
| ui.print(subtitle(format!("{}: {}", class_tags[i], message))); | ||
| } | ||
| Err(e) => { | ||
| ui.print(subtitle(format!("{}: {}", class_tags[i], e))); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Ohayo, sensei! Review of new verification functionality in walnut_verify_migration_strategy.
The implementation of the walnut_verify_migration_strategy function is robust, providing a comprehensive approach to verifying classes declared during migration. The function checks network conditions, collects source code, prepares verification payloads, and handles API interactions efficiently.
A few points to consider:
- Ensure that the API key and URL retrieval (
walnut_get_api_key,walnut_get_api_url) are secure and handle possible errors gracefully. - The use of
join_allfor running verification tasks concurrently is efficient and appropriate for this context. - Error handling and user feedback through the UI are well-implemented, providing clear and informative messages.
Consider adding more detailed logging or debugging information to help trace the verification process in case of issues. Also, ensure that all external interactions are secure and handle potential data sensitivity appropriately.
Consider enhancing logging or debugging outputs to provide more detailed insights during the verification process, which could be crucial for troubleshooting in production environments.
crates/sozo/ops/src/migration/mod.rs
Outdated
| match auto_authorize( | ||
| ws, | ||
| &world, | ||
| &txn_config, | ||
| &default_namespace, | ||
| &grant, | ||
| &revoke, | ||
| &Url::parse(&rpc_url).ok(), | ||
| ) | ||
| .await | ||
| { |
There was a problem hiding this comment.
Review the handling of the --walnut flag in auto_authorize calls.
Ohayo, sensei! It appears that the --walnut flag's effect is not explicitly managed within the auto_authorize function calls. From the previous discussions and the screenshot provided by kariy, it seems necessary to adjust how the --walnut flag is handled to avoid unintended debugging or verification during certain transactions.
Consider implementing the suggested fix by kariy to manage the --walnut flag more appropriately within these calls. Here's how you might adjust the configuration:
match auto_authorize(
ws,
&world,
&txn_config,
+ &TxnConfig { walnut: false, ..txn_config.clone() },
&default_namespace,
&grant,
&revoke,
&Url::parse(&rpc_url).ok(),
)
.awaitThis change ensures that the --walnut flag is disabled for transactions where it's not applicable, aligning with the intended use of this feature.
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.
| match auto_authorize( | |
| ws, | |
| &world, | |
| &txn_config, | |
| &default_namespace, | |
| &grant, | |
| &revoke, | |
| &Url::parse(&rpc_url).ok(), | |
| ) | |
| .await | |
| { | |
| match auto_authorize( | |
| ws, | |
| &world, | |
| &TxnConfig { walnut: false, ..txn_config.clone() }, | |
| &default_namespace, | |
| &grant, | |
| &revoke, | |
| &Url::parse(&rpc_url).ok(), | |
| ) | |
| .await | |
| { |
| let res = | ||
| auto_authorize(&ws, &world, &txn_config, &default_namespace, &grant, &revoke, &None).await; |
There was a problem hiding this comment.
Verify the integration of the new optional parameter in auto_authorize.
Ohayo, sensei! The addition of an optional parameter (&None) to the auto_authorize function call is an interesting change. It's essential to ensure that this new parameter is correctly handled within the function to avoid any unintended consequences or errors.
Consider adding tests or checks to verify that the auto_authorize function behaves as expected when this new parameter is utilized. Here's a suggestion for a test case:
#[tokio::test]
async fn test_auto_authorize_with_optional_param() {
let result = auto_authorize(&ws, &world, &txn_config, &default_namespace, &grant, &revoke, &None).await;
assert!(result.is_ok(), "auto_authorize should handle the optional parameter correctly");
}This test ensures that the function can handle the optional parameter without issues, maintaining the robustness of the migration process.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
crates/sozo/ops/src/migration/auto_auth.rs (1)
17-17: Ohayo, sensei! Some calls toauto_authorizeneed updating.The
auto_authorizefunction signature has been updated to include therpc_urlparameter. However, not all calls to this function have been updated to pass this new parameter. Please ensure the following locations are updated:
crates/sozo/ops/src/test_utils/setup.rs: The call toauto_authorizedoes not include therpc_urlparameter.crates/sozo/ops/src/migration/mod.rs: The call toauto_authorizedoes not include therpc_urlparameter.Analysis chain
Updated function signature to include
rpc_url.The addition of the
rpc_urlparameter to theauto_authorizefunction is crucial for enabling RPC URL customization in transaction configurations. This change aligns with the PR's objectives to enhance debugging capabilities. Ensure that all calls to this function are updated to pass the new parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `auto_authorize` include the new `rpc_url` parameter. # Test: Search for the function usage. Expect: Only occurrences with the new parameter. rg --type rust -A 5 $'auto_authorize'Length of output: 3070
| pub fn walnut_debug_transaction(rpc_url: &Url, transaction_hash: &Felt) -> Result<Url, Error> { | ||
| // Check if the RPC URL is not localhost | ||
| if rpc_url.host_str() != Some("localhost") && rpc_url.host_str() != Some("127.0.0.1") { | ||
| let mut url = Url::parse(WALNUT_APP_URL)?; | ||
|
|
||
| url.path_segments_mut().unwrap().push("transactions"); | ||
| url.query_pairs_mut() | ||
| .append_pair("rpcUrl", &encode(rpc_url.as_str())) | ||
| .append_pair("txHash", &format!("{transaction_hash:#066x}")); | ||
|
|
||
| Ok(url) | ||
| } else { | ||
| Err(Error::UnsupportedNetwork) | ||
| } | ||
| } |
There was a problem hiding this comment.
Review of walnut_debug_transaction function
The implementation of walnut_debug_transaction looks solid, sensei! Here are a few observations and suggestions:
- Error Handling: The function correctly handles the scenario where the RPC URL is localhost by returning an
UnsupportedNetworkerror. This is a good practice as it prevents misuse of the function in unsupported environments. - URL Construction: The use of
urlencoding::encodeto safely encode the RPC URL and the detailed construction of the transaction URL usingpath_segments_mutandquery_pairs_mutare commendable. This ensures that the URL is correctly formatted and safe from injection attacks. - Documentation: As previously noted, consider adding more detailed documentation about when and how to use this function effectively. This will aid other developers in understanding the purpose and limitations of the function.
Would you like help with drafting the documentation?
There was a problem hiding this comment.
@barabanovro great job on that, let's iterate with this first version and ensure server wise it matches on your end. We can iterate for the next version if required.
| pub fn walnut_get_api_url() -> String { | ||
| env::var(WALNUT_API_URL_ENV_VAR).unwrap_or_else(|_| WALNUT_API_URL.to_string()) | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for API URL retrieval.
The function walnut_get_api_url retrieves the Walnut API URL from an environment variable, providing a default value if the variable isn't set. While this approach avoids runtime errors, it might be beneficial to log a warning or inform the user when the default URL is being used, to ensure transparency in the application's behavior.
Consider adding a logging statement or user notification mechanism to inform when the default URL is being used. This could improve the observability of the application and help in troubleshooting configuration issues.
| match auto_authorize( | ||
| ws, | ||
| &world, | ||
| &txn_config, | ||
| &default_namespace, | ||
| &grant, | ||
| &revoke, | ||
| &walnut_debugger, | ||
| ) | ||
| .await | ||
| { |
There was a problem hiding this comment.
Review the handling of the --walnut flag in auto_authorize calls.
It appears that the --walnut flag's effect is not explicitly managed within the auto_authorize function calls. From the previous discussions and the screenshot provided by kariy, it seems necessary to adjust how the --walnut flag is handled to avoid unintended debugging or verification during certain transactions.
Consider implementing the suggested fix by kariy to manage the --walnut flag more appropriately within these calls. Here's how you might adjust the configuration:
match auto_authorize(
ws,
&world,
&txn_config,
+ &TxnConfig { walnut: false, ..txn_config.clone() },
&default_namespace,
&grant,
&revoke,
&Url::parse(&rpc_url).ok(),
)
.awaitThis change ensures that the --walnut flag is disabled for transactions where it's not applicable, aligning with the intended use of this feature.
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.
| match auto_authorize( | |
| ws, | |
| &world, | |
| &txn_config, | |
| &default_namespace, | |
| &grant, | |
| &revoke, | |
| &walnut_debugger, | |
| ) | |
| .await | |
| { | |
| match auto_authorize( | |
| ws, | |
| &world, | |
| &TxnConfig { walnut: false, ..txn_config.clone() }, | |
| &default_namespace, | |
| &grant, | |
| &revoke, | |
| &walnut_debugger, | |
| ) | |
| .await | |
| { |
Description
This PR introduces the
--walnutflag, enhancing both thesozo executeandsozo migrate applycommands:sozo executecommand: When this flag is provided, it will output a link to the Walnut page with the transaction, allowing users to debug it.sozo migrate applycommand: When this flag is provided, it will verify the classes declared during migration.Related issue
Tests
Added to documentation?
Checklist
scripts/prettier.sh,scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit
New Features
--walnutflag, enhancing the debugging capabilities of the Dojo application.WalnutDebuggerstruct for enhanced debugging and verification of Starknet transactions.Bug Fixes
Documentation
Chores