feat(dojo-lang): add warnings for migrations with prints

[jsanchez556]

Description

Related issue

Tests

• Yes
• No, because they aren't needed
• No, because I need help

Added to documentation?

• README.md
• Dojo Book
• No documentation needed

Checklist

• I've formatted my code (scripts/prettier.sh, scripts/rust_fmt.sh, scripts/cairo_fmt.sh)
• I've linted my code (scripts/clippy.sh, scripts/docs.sh)
• I've commented my code
• I've requested a review after addressing the comments

Summary by CodeRabbit

• New Features

  • Introduced a new dependency for improved string manipulation capabilities.
  • Enhanced contract management during the build process, including checks for unsupported functions.
  • Added new functionality for handling file paths, with user-friendly error messaging.
  • Introduced new modules for better organization of functionalities within the project.
  • Added a new struct to enhance file path manipulation capabilities.

• Bug Fixes

  • Improved error handling and reporting for unsupported library functions during contract compilation.

[coderabbitai]

Walkthrough

Ohayo, sensei! This change integrates the dunce crate as a new dependency for string manipulation in the Cargo.toml files across multiple crates. It enhances the DojoCompiler with improved validation for allowed library functions and refines error handling during contract compilation. Additionally, new modules and structures have been established for better file path management, enhancing the organization and functionality of the codebase.

Changes

File	Change Summary
Cargo.toml, crates/dojo-lang/Cargo.toml	Added dunce dependency for string manipulation in the main and dojo-lang crates.
crates/dojo-lang/src/compiler.rs	Enhanced DojoCompiler with new fields in Props struct for managing allowed library functions; added SerdeListSelector enum for specifying allowed functions. Updated imports to include NamedLanguageElementId.
crates/dojo-lang/src/scarb_internal/fsx.rs	Added canonicalize and canonicalize_utf8 functions for improved path handling; introduced PathBufUtf8Ext trait for PathBuf conversion.
crates/dojo-lang/src/scarb_internal/mod.rs	Added new public modules serdex and fsx for organizing related functionalities.
crates/dojo-lang/src/scarb_internal/serdex.rs	Introduced RelativeUtf8PathBuf struct with methods for generating relative paths, enhancing path manipulation capabilities.

Sequence Diagram(s)

sequenceDiagram
    participant Developer
    participant Compiler
    participant ContractValidator
    participant PathHandler

    Developer->>Compiler: Compile contract
    activate Compiler
    Compiler->>ContractValidator: Validate library functions
    activate ContractValidator
    ContractValidator-->>Compiler: Return validation results
    deactivate ContractValidator

    Compiler->>PathHandler: Resolve file paths
    activate PathHandler
    PathHandler-->>Compiler: Return canonicalized paths
    deactivate PathHandler

    Compiler-->>Developer: Compile result
    deactivate Compiler

Loading

---

Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 84cff91 and f6e5465.

Files selected for processing (1)

• crates/dojo-lang/src/compiler.rs (2 hunks)

Files skipped from review as they are similar to previous changes (1)

• crates/dojo-lang/src/compiler.rs

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[glihm]

@jsanchez556 is the error message always containing the print keyword? I'm not sure of this, would you have some reference/examples?

[jsanchez556]

@glihm

@jsanchez556 is the error message always containing the print keyword? I'm not sure of this, would you have some reference/examples?

I initially used this approach before our conversation two days ago. However, I'm now stuck trying to debug a scenario locally where contracts with print statements are being deployed. It seems like I might be doing something wrong, as neither Sozo nor Katana are throwing errors when this occurs. At this point, I'm unsure if the changes need to be made in Katana rather than Sozo. What do you think?

Steps I'm taking to try to reproduce the scenario:

1. Run Katana locally using cargo run --bin katana.
2. Add some print statements in the file located at examples/spawn-and-move/actions.cairo.
3. Compile the examples/spawn-and-move using sozo build.
4. Migrate the examples by running sozo locally: ./target/release/sozo migrate apply --manifest-path examples/spawn-and-move/Scarb.toml -vvv.

[glihm]

@glihm

@jsanchez556 is the error message always containing the print keyword? I'm not sure of this, would you have some reference/examples?

I initially used this approach before our conversation two days ago. However, I'm now stuck trying to debug a scenario locally where contracts with print statements are being deployed. It seems like I might be doing something wrong, as neither Sozo nor Katana are throwing errors when this occurs. At this point, I'm unsure if the changes need to be made in Katana rather than Sozo. What do you think?

Steps I'm taking to try to reproduce the scenario:

1. Run Katana locally using cargo run --bin katana.
2. Add some print statements in the file located at examples/spawn-and-move/actions.cairo.
3. Compile the examples/spawn-and-move using sozo build.
4. Migrate the examples locally: ./target/release/sozo migrate apply --manifest-path examples/spawn-and-move/Scarb.toml -vvv.

The thing is that on Katana, it's totally ok to use print. But it's when one wants to migrate the project on a public network. This will cause the declaration to fail because print belongs to a family of function that is not accepted on the network.

For this reason I was mentioning that you can't rely on an error message to know if contracts contain print.
In the compiler, we also generates the expanded source code of the contracts. You may use this step to actually check for print statements (which are allowed in tests though as they are not deployed).

dojo/crates/dojo-lang/src/compiler.rs

Line 367 in 8db91e7

save_expanded_source_file(

At this point, you may run a check on the source code, if a print statement is found outside of the tests, you can emit a warning.

Or you can do it in the sozo build command, inspecting the files after they are generated by the compiler (may be easier this approach, as you already have the manifests path, and you can simply iterate on the files).

[jsanchez556]

@glihm

@jsanchez556 is the error message always containing the print keyword? I'm not sure of this, would you have some reference/examples?

I initially used this approach before our conversation two days ago. However, I'm now stuck trying to debug a scenario locally where contracts with print statements are being deployed. It seems like I might be doing something wrong, as neither Sozo nor Katana are throwing errors when this occurs. At this point, I'm unsure if the changes need to be made in Katana rather than Sozo. What do you think?
Steps I'm taking to try to reproduce the scenario:

1. Run Katana locally using cargo run --bin katana.
2. Add some print statements in the file located at examples/spawn-and-move/actions.cairo.
3. Compile the examples/spawn-and-move using sozo build.
4. Migrate the examples locally: ./target/release/sozo migrate apply --manifest-path examples/spawn-and-move/Scarb.toml -vvv.

The thing is that on Katana, it's totally ok to use print. But it's when one wants to migrate the project on a public network. This will cause the declaration to fail because print belongs to a family of function that is not accepted on the network.

For this reason I was mentioning that you can't rely on an error message to know if contracts contain print. In the compiler, we also generates the expanded source code of the contracts. You may use this step to actually check for print statements (which are allowed in tests though as they are not deployed).

dojo/crates/dojo-lang/src/compiler.rs

Line 367 in 8db91e7

save_expanded_source_file(

At this point, you may run a check on the source code, if a print statement is found outside of the tests, you can emit a warning.

Or you can do it in the sozo build command, inspecting the files after they are generated by the compiler (may be easier this approach, as you already have the manifests path, and you can simply iterate on the files).

@glihm could you please check this PR

[coderabbitai]

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (1)

bin/sozo/src/commands/build.rs (1)

87-97: Enhance the warning message for unsupported print functions.

The current warning message informs the user about the presence of print functions but could be more helpful by suggesting how to locate and remove them.

Consider updating the message as follows:

ui.print(
    "Warning: Some contracts include `print` functions that are not supported on the current network. \
    Please locate and remove these functions from your contract files to ensure compatibility. \
    You can search for `print` in the expanded source code or the compiled JSON files.",
);

[coderabbitai]

Actionable comments posted: 0

Outside diff range, codebase verification and nitpick comments (2)

bin/sozo/src/utils.rs (2)

141-155: Ohayo, sensei! Enhance error handling for file extensions.

Using unwrap_or_default for file extensions can lead to unexpected behavior if the extension is missing. Consider using a more robust error handling approach.

Here's a suggested refactor:

fn get_files_from_dir(dir: &Utf8PathBuf, extension: &str) -> Vec<PathBuf> {
    let mut files = Vec::new();

    if let Ok(entries) = fs::read_dir(dir) {
        for entry in entries {
            if let Ok(path) = entry.map(|e| e.path()) {
                if path.is_file() {
                    if let Some(ext) = path.extension() {
                        if ext == extension {
                            files.push(path);
                        }
                    }
                }
            }
        }
    }

    files
}

---

157-173: Ohayo, sensei! Enhance error handling for file paths.

Using unwrap_or_default for file paths can lead to unexpected behavior if the path conversion fails. Consider using a more robust error handling approach.

Here's a suggested refactor:

pub fn exists_func_in_contracts(target_dir: &Utf8PathBuf, functions: &[&str]) -> bool {
    let compiled_contracts = get_files_from_dir(target_dir, "json");
    let mut type_exists = false;

    for contract in compiled_contracts {
        if let Some(contract_str) = contract.to_str() {
            if let Ok(file_content) = fs::read_to_string(contract_str) {
                type_exists = functions
                    .iter()
                    .any(|&func| file_content.contains(&format!("\"{}\"", func)));

                if type_exists {
                    break;
                }
            }
        }
    }

    type_exists
}

[glihm]

@jsanchez556 nice work on that! Looking into the Sierra is a very good approach.

This code should work, I'm just wondering about the potential performance running contains on Sierra files that may be big.

The print is found under sierra_program_debug_info, we may restrict the contains to this part.

Also, checking on scarb, they use this function: https://github.com/software-mansion/scarb/blob/b29b842af42df38ee118eaceb0bea88da35dbab4/scarb/src/compiler/compilers/starknet_contract.rs#L414
As we already have the ContractDeclaration and ContractClass in memory, it might be more efficient.

WDYT? Could be great if you could give it a try. The scarb function is not public, so we may have to implement a similar logic though.

[jsanchez556]

@jsanchez556 nice work on that! Looking into the Sierra is a very good approach.

This code should work, I'm just wondering about the potential performance running contains on Sierra files that may be big.

The print is found under sierra_program_debug_info, we may restrict the contains to this part.

Also, checking on scarb, they use this function: https://github.com/software-mansion/scarb/blob/b29b842af42df38ee118eaceb0bea88da35dbab4/scarb/src/compiler/compilers/starknet_contract.rs#L414 As we already have the ContractDeclaration and ContractClass in memory, it might be more efficient.

WDYT? Could be great if you could give it a try. The scarb function is not public, so we may have to implement a similar logic though.

@glihm I've implemented a similar logic based on scarb. Could you please review it and share your thoughts?

[glihm]

@jsanchez556 good work!
In the case of Dojo, we can have a simplified version for now, please find the comments below, and if you have any comment / other idea, please don't hesitate!

Thank you again for the work done here.

[jsanchez556]

@coderabbitai full review

[codecov]

Codecov Report

Attention: Patch coverage is 26.31579% with 14 lines in your changes missing coverage. Please review.

Project coverage is 67.24%. Comparing base (a3ddf39) to head (f6e5465).
Report is 1 commits behind head on main.

Files	Patch %	Lines
crates/dojo-lang/src/compiler.rs	26.31%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2316      +/-   ##
==========================================
- Coverage   67.26%   67.24%   -0.02%     
==========================================
  Files         357      357              
  Lines       46634    46657      +23     
==========================================
+ Hits        31367    31376       +9     
- Misses      15267    15281      +14     

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

[glihm]

Awesome @jsanchez556 thanks for the work done here and your time to contribute! 🚀