Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add missing contract tag in logs #2551

Merged
merged 1 commit into from
Oct 17, 2024
Merged

add missing contract tag in logs #2551

merged 1 commit into from
Oct 17, 2024

Conversation

notV4l
Copy link
Collaborator

@notV4l notV4l commented Oct 17, 2024

Add missing contract tag in sozo migrate logs

Summary by CodeRabbit

  • New Features

    • Enhanced clarity in migration messages by including contract tags in outputs.
    • Improved performance in model declaration handling with concurrent processing.
  • Bug Fixes

    • Refined error handling for contract deployment and upgrades, providing more descriptive error messages.
  • User Interface Improvements

    • Enhanced user feedback during the migration process for a better user experience.
  • Chores

    • Updated functions to ensure robust metadata uploading and namespace registration.
    • Ensured deployment manifests are updated with the latest transaction details.

@notV4l notV4l requested a review from glihm October 17, 2024 12:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (6)
crates/sozo/ops/src/migration/migrate.rs (6)

Line range hint 1060-1064: Clarify Handling When Class Hashes Match

Ohayo, sensei! In the get_contract_operation_name function, when the current_class_hash equals contract.diff.local_class_hash, the code currently returns "{}: Already Deployed". To enhance clarity, consider explicitly handling the case where the class hashes match, indicating that the contract is already up-to-date.

Consider modifying the match arms to explicitly handle the matching class hash:

 match provider.get_class_hash_at(BlockId::Tag(BlockTag::Pending), contract_address).await {
     Ok(current_class_hash) if current_class_hash != contract.diff.local_class_hash => {
         format!("{}: Upgrade", contract.diff.tag)
+    }
+    Ok(current_class_hash) if current_class_hash == contract.diff.local_class_hash => {
+        format!("{}: Already Up-to-Date", contract.diff.tag)
     }
     Err(ProviderError::StarknetError(StarknetError::ContractNotFound)) => {
         format!("{}: Deploy", contract.diff.tag)
-    }
-    Ok(_) => format!("{}: Already Deployed", contract.diff.tag),
+    }
+    Err(_) => format!("{}: Deploy", contract.diff.tag),
 }

Line range hint 541-600: Refactor Asynchronous Task Distribution for Readability

Ohayo, sensei! The register_dojo_models_with_declarers function involves complex asynchronous task distribution among declarers. The nested loops and HashMap usage make the code a bit intricate. Refactoring this section could improve readability and maintainability.

Consider the following refactoring suggestions:

  • Extract Functions: Break down the task distribution and result handling into separate helper functions.
  • Use Concise Iterators: Utilize Rust's iterator methods like enumerate() and map() for cleaner loops.
  • Simplify Async Logic: Consider using try_join_all directly with the tasks to manage asynchronous execution.

Line range hint 1278-1285: Ensure Metadata Upload Errors are Handled Properly

Ohayo, sensei! In the upload_metadata function, when uploading IPFS artifacts, errors are caught and printed, but the process continues. This might lead to incomplete metadata registration without clear failure signaling.

Consider handling errors more robustly:

  • If an upload fails, decide whether to halt the process or collect all failures and report them at the end.
  • Ensure that the resources vector accurately reflects only the successfully uploaded metadata.

Line range hint 408-452: Handle Artifact Errors with Specific Information

Ohayo, sensei! In the register_dojo_models function, when handling ArtifactError, the error message recommends running sozo clean and sozo build. Providing more context about the error can help users resolve issues faster.

Enhance the error message with specific details:

-anyhow!(
-    "Discrepancy detected in {name}.\nUse `sozo clean` to clean your project.\n
-    Then, rebuild your project with `sozo build`."
-)
+anyhow!(
+    "Discrepancy detected in {name}: {error}.\nPlease run `sozo clean` to clean your project, \
+then rebuild with `sozo build` to resolve the issue."
+)

Line range hint 279-288: Improve Error Messages in Contract Deployment

Ohayo, sensei! In the deploy_contract function, when deployment fails, the error message could provide more actionable information.

Consider including the contract ID and more details in the error message:

 Err(e) => {
     ui.verbose(format!("{e:?}"));
-    Err(anyhow!("Failed to migrate {contract_id}: {e}"))
+    Err(anyhow!("Failed to deploy contract '{contract_id}': {e}. Please check the contract configuration and try again."))
 }

Line range hint 151-168: Avoid Duplicate Code in Contract Registration Functions

Ohayo, sensei! The register_dojo_models_with_declarers and register_dojo_contracts_declarers functions have similar logic to their counterparts without declarers.

To follow the DRY (Don't Repeat Yourself) principle, consider refactoring:

  • Abstract common logic into shared helper functions.
  • Use generics or traits if necessary to handle different types.
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b557963 and 560c5d3.

📒 Files selected for processing (1)
  • crates/sozo/ops/src/migration/migrate.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (2)
crates/sozo/ops/src/migration/migrate.rs (2)

Line range hint 608-635: Optimize Namespace Registration

Ohayo, sensei! In the register_namespaces function, fetching and checking each namespace individually could be optimized.

[performance]

Consider batching the resource checks to reduce the number of asynchronous calls:

  • Fetch all namespace resources in one batch call if the API supports it.
  • Filter out registered namespaces more efficiently to minimize the number of iterations.

Line range hint 240-257: Check for Missing Namespace Registration

Ohayo, sensei! Before registering models and contracts, ensure that all required namespaces have been registered to prevent potential deployment issues.

Run the following script to verify namespace registration:

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 69.55%. Comparing base (70ecca7) to head (560c5d3).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
crates/sozo/ops/src/migration/migrate.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2551   +/-   ##
=======================================
  Coverage   69.55%   69.55%           
=======================================
  Files         388      388           
  Lines       49964    49964           
=======================================
  Hits        34750    34750           
  Misses      15214    15214           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@glihm glihm merged commit 43e2aec into main Oct 17, 2024
14 of 15 checks passed
@glihm glihm deleted the fix_sozo_logs branch October 17, 2024 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants